[GitHub] [geode-native] albertogpz opened pull request #556: GEODE-7509: Fix memory leaks in C++ client

["albertogpz (GitHub)" <gi...@apache.org>]

This commit solves some more memory leaks
and uninitialized memory accesses found
in the C++ client when running the integration
tests that were not solved by GEODE-7476


[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #556: GEODE-7509: Fix memory leaks in C++ client

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

`auto` please

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #556: GEODE-7509: Fix memory leaks in C++ client

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

This isn't your problem but maybe you want to tackle it.
I dug a little deeper in here. It felt wrong that we were just reading the raw bytes into the string. As it turns out my gut was correct. The server either sends an ASCII only string (by setting of global flag that defaults false) or Java Modified UTF-8 (see `Message.addStringPart`). Since Java Modified UTF-8 isn't a standard we have special code on the C++ client to transcode it to UTF-8. This block of code should really be invoking a call to read Java Modified UTF-8. It would be great if we could just invoke it right on `DataInput` but the method that can read JMUTF-8 is private and expects to read the length too but when encode in `Part` the length is in the part header not the body. I think the solution is to just read the JMUTF directly using `JavaModifiedUtf8::decode`, which gives you the `std::u16string` (easy transcode), then transcode that to `std::string` in UTF8 using `to_utf8` in util/string.hpp. 

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on issue #556: GEODE-7509: Fix memory leaks in C++ client

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

> If you have a suggestion to make this code more clear I would be willing to improve it.

The overall plan is to remove ACE library so these handlers will go away with that. I would just put in the fix as is for now.

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #556: GEODE-7509: Fix memory leaks in C++ client

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Ok, I can reproduce the failure with string literal `u8"SimpleCQ\xF0\x90\x90\x80"` as the CQ name. This UTF-8 sequence produces the U+10400 character "𐐀", which has two UTF-16 code points and results in 6 bytes of JMUTF-8 rather then 4 bytes of UTF-8.

I also found a race condition in the new CqTest but I will write that up separately.


[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] albertogpz commented on issue #556: GEODE-7509: Fix memory leaks in C++ client

["albertogpz (GitHub)" <gi...@apache.org>]

I forgot to ask something that did not really feel right on one of the fixes.
I noticed I had to add a `delete this` sentence in the `handle_close()` method of the different ExpiryHandler classes in order to remove the leaking of the memory of the handlers in spite the fact there was a comment stating that the handler would be deleted now in `GF_Timer_Heap_ImmediateReset_T`. It seems that the handler was not deleted by that class in all cases.
Apart from that, the `TombstoneExpiryHandler.handle_close()` method did not require such delete sentence because these handlers were deleted in `TombstoneList::cleanUp()`.
If you have a suggestion to make this code more clear I would be willing to improve it.

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] albertogpz commented on pull request #556: GEODE-7509: Fix memory leaks in C++ client

["albertogpz (GitHub)" <gi...@apache.org>]

Right

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] albertogpz commented on pull request #556: GEODE-7509: Fix memory leaks in C++ client

["albertogpz (GitHub)" <gi...@apache.org>]

Interesting...

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] albertogpz commented on issue #556: GEODE-7509: Fix memory leaks in C++ client

["albertogpz (GitHub)" <gi...@apache.org>]

I forgot to ask something that did not really feel right on one of the fixes.
I noticed I had to add a `delete this` sentence in the `handle_close()` method of the different ExpiryHandler classes in order to remove the leaking of the memory of the handlers in spite the fact there was a comment stating that the handler would be deleted now in `GF_Timer_Heap_ImmediateReset_T`. It seems that the handler was not deleted by that class in all cases.
Apart from that, the `TombstoneExpiryHandler.handle_close()` method did not require such delete sentence because these handlers were deleted in `TombstoneList::cleanUp()`.
If you have a suggestion to make this code clear I would be willing to improve it.

[ Full content available at: https://github.com/apache/geode-native/pull/556 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org