> On April 28, 2015, 10:08 p.m., Rafael Schloming wrote: > > proton-c/src/tests/object.c, line 804 > > <https://reviews.apache.org/r/33623/diff/3/?file=944252#file944252line804> > > > > The pn_string_t type is mutable, so the hash you are setting up here > > has a lot of different keys all pointing to a single mutable string value. > > > > The reason this appears to manifest as a valgrind error is that your > > assertion checking the value associated with each key is failing since all > > the keys map to the same value. When this happens the map (and string > > value) are not freed. The relevant error though is not the memory leak but > > the assertion failure. > > > > This is a bit hard to spot in the test output since the valgrind output > > is noiser, but the process is also aborting with the assert failure. > > > > It occurs to me if we install a handler for the abort signal and print > > a stack trace then it would be a lot more obvious what is going on here.
Sorry, I had indeed missed that. Fixed now though. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33623/#review81890 ----------------------------------------------------------- On April 29, 2015, 11:53 a.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33623/ > ----------------------------------------------------------- > > (Updated April 29, 2015, 11:53 a.m.) > > > Review request for qpid, Alan Conway, Andrew Stitcher, and Rafael Schloming. > > > Bugs: PROTON-858 > https://issues.apache.org/jira/browse/PROTON-858 > > > Repository: qpid-proton-git > > > Description > ------- > > The internal map implementation for proton-c uses coalesced hashing, but the > current deletion algorithm does not take account of this. On deletion of > entries, the map can lose its ability to locate other entries by key. > > This patch is the simplest possible fix. It frees the deleted entry, makes > any previous entry that points to this tha tail of its chain, and then > relocates any entries in a chain following the deleted entry. Though I > believe this to be correct and safe, it does involve more work on some > deletions. The approach can be optimised further, either be reducing the > amount of chain that needs relocated, or by marking records as deleted in the > first instance and only rehashing later on some criteria. > > (I started off trying to do the latter, but it is more complicated and I > still haven't got my patch fully correct. When I get that working I'll post > that also. It is a more involved change though, so perhaps we may want to > consider this simpler approach for 0.9.1). > > > Diffs > ----- > > proton-c/src/object/map.c 1a758a1 > proton-c/src/tests/object.c 2b213c5 > > Diff: https://reviews.apache.org/r/33623/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
