Ticket #11467 (closed: fixed)
Fix memory leaks from parsing compute resource info
| Reported by: | Martyn Gigg | Owned by: | Federico M Pouzols |
|---|---|---|---|
| Priority: | critical | Milestone: | Release 3.4 |
| Component: | Framework | Keywords: | |
| Cc: | Blocked By: | ||
| Blocking: | Tester: | Harry Jeffery |
Description
Valgrind has spotted some memory leaks with the compute resource parsing code: http://builds.mantidproject.org/view/Valgrind/job/valgrind_core_packages/369/valgrindResult/
Two things are worth noting:
ComputeResourceInfoTest.h:66 - test_normalFermi(): fac not deleted - http://builds.mantidproject.org/view/Valgrind/job/valgrind_core_packages/369/valgrindResult/pid=4462,0x1c/
ComputeResourceInfo.cpp:50 - Poco docs (http://www.appinf.com/docs/poco/Poco.XML.Node.html#39829) say that value returned from childNodes must be released - http://builds.mantidproject.org/view/Valgrind/job/valgrind_core_packages/369/valgrindResult/pid=4462,0x20/
Change History
comment:2 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from new to inprogress
prevent unreleased Poco XML object leak, re #11467
Changeset: fdef668c0a87cecc1f796202f9537b200a26de5a
comment:3 Changed 6 years ago by Federico Montesino Pouzols
use shared_ptr to prevent leaks, re #11467
Changeset: 9e5fea3913705328edd68f133d6acf9ccbb17d88
comment:4 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from inprogress to verify
- Resolution set to fixed
This is being verified as pull request #508.
comment:5 Changed 6 years ago by Federico Montesino Pouzols
used shared_ptr def constructor to make msvc happy, re #11467
Changeset: 7df52ce5fe955f1bca31cbb6d6a9c3c54fad384a
comment:6 Changed 6 years ago by Federico Montesino Pouzols
In the last build everything went fine, except that SystemTests.CodeConventions complains about an unrelated algorithm. So despite the red cross I'd say this is ready for testing.
comment:9 Changed 6 years ago by Stuart Campbell
@FedeMPouzols I presume you haven't seen @rosswhitfield comments on slack - the old plugin is now disabled so the retest this type comments won't work anymore. You can either restart the job on jenkins directly or they will trigger with any new code changes.
comment:10 Changed 6 years ago by Federico Montesino Pouzols
Jenkins, retest this please
comment:11 Changed 6 years ago by Pete Peterson
I merged master back into the branch to kick off a new round of builds
comment:12 Changed 6 years ago by Federico Montesino Pouzols
Good, thanks! One of them was definitely stuck since yesterday.
comment:13 Changed 6 years ago by Harry Jeffery
- Status changed from verify to verifying
- Tester set to Harry Jeffery
comment:14 Changed 6 years ago by Harry Jeffery
The only change has been to the ComputeResourceInfo unit test, which has no passed on all platforms. I'm not going to bother to wait for RHEL7 or Windows to finish as any failures that occur can only be random and unrelated.
Reviewed the code and it looks good to me.
comment:15 Changed 6 years ago by Harry Jeffery
- Status changed from verifying to closed
Merge pull request #508 from mantidproject/11467_fix_leaks_in_ComputeResourInfo_XML_parsing
Fix leaks in ComputeResourceInfo (XML parsing and test)
Full changeset: 56417500b7690056be2ce2bdbf26c456dd16207d
comment:16 Changed 5 years ago by Nick Draper
Somehow these slipped through without a resolution. Set to Fixed.
comment:17 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 12306
From the Poco docs I would suggest using nl->item(0)->hasChildNodes() instead as it would be cleaner