Ticket #8153 (closed: fixed)
Memory leak in CatalogInfo
Reported by: | Russell Taylor | Owned by: | Jay Rainey |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.0 |
Component: | Framework | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Russell Taylor |
Description
Our automated valgrind job (https://builds.sns.gov/job/ornl_valgrind_develop/) recently showed a big jump in the number of leaks detected. Unfortunately, the plugin isn't too helpful in pinpointing why but I suspect it may be at least in part from CatalogInfo.cpp line 150, which showed up in a manual valgrind run I just did and is also run every time the ConfigService starts up.
From the Poco documentation for getElementsByTagName: "The returned NodeList must be released with a call to release() when no longer needed." You should probably use a Poco::AutoPtr to handle this.
Do check your code for other places the same problem may exist.
Change History
comment:2 Changed 7 years ago by Jay Rainey
Release the tag. Refs #8153.
- It appears I only call release() if the elementTag == 1. I should call it otherwise to. This will prevent the memory leak.
Changeset: bd0e150e36ae9172dad4100b4e44232c413013fd
comment:3 Changed 7 years ago by Jay Rainey
- Status changed from inprogress to verify
- Resolution set to fixed
comment:4 Changed 7 years ago by Russell Taylor
- Status changed from verify to verifying
- Tester set to Russell Taylor
comment:5 Changed 7 years ago by Russell Taylor
- Status changed from verifying to reopened
- Resolution fixed deleted
Running the valgrind job again shows that this was the source of the jump in the number of leaks. However, please just change line 150 to be Poco::AutoPtr<Poco::XML::NodeList> elementTag = element->getElementsByTagName(tagName);. That way, you do not need to worry about where to call release() at all, it will be called when the variable goes out of scope (and it's exception-safe). Your current change of adding release twice leads to invalid read/write warnings in valgrind.
comment:7 Changed 7 years ago by Jay Rainey
Auto pointer for nodelist. Refs #8153.
Changeset: e17dcad92e130e7692e71ce08730aa89342de208
comment:8 Changed 7 years ago by Jay Rainey
- Status changed from inprogress to verify
- Resolution set to fixed
comment:10 Changed 7 years ago by Russell Taylor
- Status changed from verifying to closed
Merge remote branch 'origin/feature/8153_cataloginfo_memory_leak'
Full changeset: 23a26ab9de9f2bf29d6357d1b24035889a32ff78
comment:11 Changed 7 years ago by Jay Rainey
I published code to the wrong ticket. Have fixed this now. See #8152.
comment:12 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 8998
.