Ticket #11467 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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:1 Changed 6 years ago by Martyn Gigg

From the Poco docs I would suggest using nl->item(0)->hasChildNodes() instead as it would be cleaner

Last edited 6 years ago by Martyn Gigg (previous) (diff)

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:7 Changed 6 years ago by Federico Montesino Pouzols

Jenkins, retest this please

comment:8 Changed 6 years ago by Federico Montesino Pouzols

Jenkins, wake up and retest this please

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

Note: See TracTickets for help on using tickets.