Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17110 )
Change subject: IMPALA-10450: Catalogd crashes due to exception in ThriftDebugString ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/17110/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17110/6//COMMIT_MSG@13 PS6, Line 13: caught on the catalogd side and it crashes the catalogd. > Will the thrift library has memory leak or corrupt memory after this? It'd the code in thrift which expands the memory buffer tries to allocate first than then assign to the pointer as seen here: https://github.com/apache/thrift/blob/53dd39833a08ce33582e5ff31fa18bb4735d6731/lib/cpp/src/thrift/transport/TBufferTransports.cpp#L371 Based on my understanding, realloc will only reassign the pointer if it is able to allocate the memory otherwise it returns a nullptr and doesn't modify the original pointer (ref: https://en.cppreference.com/w/cpp/memory/c/realloc) Since the TMemoryBuffer is defined as a local variable in ThriftDebugString here (https://github.com/apache/thrift/blob/6140fb27e0b15de2ba042401073435b049482389/lib/cpp/src/thrift/protocol/TDebugProtocol.h#L162) when a exception is thrown, the memory buffer is freed up by the destructor as defined here https://github.com/apache/thrift/blob/6140fb27e0b15de2ba042401073435b049482389/lib/cpp/src/thrift/transport/TBufferTransports.h#L565. So think we should not have a leak or corrupt memory. I will see if I can reproduce this somehow. If you have a any ideas, I can try them out. One thing which I tried is I updated the thrift library to throw a exception and I looped around on the catalogd web API call for an hour. I observed the memory usage of catalogd and it was stable. http://gerrit.cloudera.org:8080/#/c/17110/6/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/17110/6/be/src/util/debug-util.h@196 PS6, Line 196: EXCEPTION_STR_MAP > nit: EXCEPTION_STR_MAP Done http://gerrit.cloudera.org:8080/#/c/17110/6/be/src/util/thrift-debug-util.h File be/src/util/thrift-debug-util.h: http://gerrit.cloudera.org:8080/#/c/17110/6/be/src/util/thrift-debug-util.h@33 PS6, Line 33: > nit: I think we can add 'noexcept' here Done -- To view, visit http://gerrit.cloudera.org:8080/17110 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42cee6186a3d5bacc1117bae5961ac60ac9f7a66 Gerrit-Change-Number: 17110 Gerrit-PatchSet: 7 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Fri, 26 Feb 2021 01:01:41 +0000 Gerrit-HasComments: Yes
