Conversation
|
I've stumbled upon the exact same thing recently but I haven't found time to publish it. I solved it a bit different: |
|
Thank you very much for your contribution @mgrrx. As far as I know, Nevertheless, I think it is about time migrating to smart pointers from raw pointers to automate memory management in such a crucial package as this one. Especially in applications that run on-board nodes with distant missions (e.g. a UAV in a SAR mission), this might result in critical failures. |
|
I changed the target branch to During the initialization the appender is also registered with rosconsole: ros_comm/clients/roscpp/src/libros/init.cpp Line 342 in 53d958f In order to safely delete this it should also be unregistered before. |
Delete g_rosout_appender explicitly instead of assigning it to NULL. Follow deletion with NULL assignment.
1e01077 to
db24dc9
Compare
|
@dirk-thomas I couldn't find any function call for de-registration either in ros::console or ros::console::impl. Could you point to a specific location if there exists one? |
There might not be one yet? |
|
OK, I will add a deregistry function to rosconsole repository soon. |
|
Please update the package manifest that the next upcoming patch release of |
|
I have updated the version number of ros_comm. I don't know a way to specify a release as the dependency in the package.xml file. If I have made anything wrong or insufficient, please correct me. |
ros_comm/package.xml
Outdated
| <package> | ||
| <name>ros_comm</name> | ||
| <version>1.14.3</version> | ||
| <version>1.14.4</version> |
There was a problem hiding this comment.
Sorry, I should have been more specific. Please revert this change and instead modify the file clients/roscpp/package.xml and replace these two lines:
<build_depend>rosconsole</build_depend>
<run_depend>rosconsole</run_depend>
with:
<build_depend version_gte="1.13.8">rosconsole</build_depend>
<run_depend version_gte="1.13.8">rosconsole</run_depend>
This basically expresses that roscpp needs a newer version of rosconsole since it requires the new function to exist. [CI will fail until a new release of rosconsole is being made.]
This reverts commit 795c8fd.
dirk-thomas
left a comment
There was a problem hiding this comment.
LGTM - thank you for working on this.
CI will need to be rerun after rosconsole has been released.
|
@ros-pull-request-builder retest this please |
|
Hi, did this time out? |
|
@ros-pull-request-builder retest this please |
|
I retriggered the PR jobs since the previous builds have been removed due to the age. The latest builds show numerous test failures. |
|
I believe that is the result of changes in the upstream. I will try to have a stable build with current upstream + my changes. |
|
I have found the problem. Line at https://github.com/ros/rosconsole/blob/ba4e1ae06b5bce3039ac5d20853248b51198e431/src/rosconsole/impl/rosconsole_log4cxx.cpp#L365 has an inherent assumption on the registration of the appender. |
|
|
||
| ros::console::deregister_appender(g_rosout_appender); | ||
| delete g_rosout_appender; | ||
| g_rosout_appender = 0; |
There was a problem hiding this comment.
Maybe try wrapping these three lines in if (g_rosout_appender) { ... }.
There was a problem hiding this comment.
Well, it won't work since g_rosout_appender is a valid pointer. However, in rosconsole side g_log4cxx_appender remains NULL.
There was a problem hiding this comment.
I'm preparing to submit a PR for non-NULL check of g_log4cxx_appender in rosconsole repository. However, it is a little bit awkward while g_rosout_appender is not NULL and g_log4cxx_appender is NULL. Is it the correct behavior?
Ok, I have found where g_log4cxx_appender is zeroed. It is assigned to NULL in the shutdown() function of rosconsole::log4cxx, which is called before deregistration. What is necessary is to remove that line. It requires the rebuild of rosconsole repository.
There was a problem hiding this comment.
From reading this file g_rosout_appender is only set when also being passed to register_appender (within the start() function). Therefore I think it needs a detailed stacktrace when / why that is the case.
There was a problem hiding this comment.
Hi again Dirk,
I have inspected shutdown and deregister_appender functions in rosconsole. Following are the functions and my diagnosis:
void shutdown()
{
if(g_log4cxx_appender)
{
const log4cxx::LoggerPtr& logger = log4cxx::Logger::getLogger(ROSCONSOLE_ROOT_LOGGER_NAME);
logger->removeAppender(g_log4cxx_appender);
g_log4cxx_appender = 0; // **Nullification**
}
// reset this so that the logger doesn't get crashily destroyed
// again during global destruction.
//
// See https://code.ros.org/trac/ros/ticket/3271
//
log4cxx::Logger::getRootLogger()->getLoggerRepository()->shutdown();
}
,
void deregister_appender(LogAppender* appender){
if(g_log4cxx_appender->getAppender() == appender) //** Trying NULL->getAppender() --> SEGFAULT **/
{
const log4cxx::LoggerPtr& logger = log4cxx::Logger::getLogger(ROSCONSOLE_ROOT_LOGGER_NAME);
logger->removeAppender(g_log4cxx_appender);
delete g_log4cxx_appender;
g_log4cxx_appender = 0;
}
}
In my PR, I was trying to call deregister_appender after shutdown, which directly results in a Segmentation Fault that makes the tests fail. In fact, I have implemented this feature in here and here. Since all the tests had passed, I thought there was no problem. However shutdown() and deregister_appender() results in segmentation fault in any context when used subsequently.
Furthermore, logger->removeAppender(g_log4cxx_appender); already deletes g_log4cxx_appender. Therefore deregister_appender is problematic since the beginning.
So, we don't need to intervene in rosconsole level. What is necessary is to just delete g_rosout_appender in init.cpp and then nullify it according to conventions.
There was a problem hiding this comment.
Finally, the result of valgrind --leak-check=full ./devel_isolated/test_roscpp/lib/test_roscpp/test_roscpp-left_right is below:
==5754== Memcheck, a memory error detector
==5754== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==5754== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==5754== Command: ./devel_isolated/test_roscpp/lib/test_roscpp/test_roscpp-left_right
==5754==
[ INFO] [1555272531.408215746]: ping!
[ INFO] [1555272531.886234315]: ping!
[ INFO] [1555272532.383400126]: ping!y los
[ INFO] [1555272532.883404174]: ping!
[ INFO] [1555272533.383366457]: ping!
==5754==
==5754== HEAP SUMMARY:
==5754== in use at exit: 3,457 bytes in 44 blocks
==5754== total heap usage: 2,016 allocs, 1,972 frees, 296,086 bytes allocated
==5754==
==5754== LEAK SUMMARY:
==5754== definitely lost: 0 bytes in 0 blocks
==5754== indirectly lost: 0 bytes in 0 blocks
==5754== possibly lost: 0 bytes in 0 blocks
==5754== still reachable: 3,457 bytes in 44 blocks
==5754== suppressed: 0 bytes in 0 blocks
==5754== Reachable blocks (those to which a pointer was found) are not shown.
==5754== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==5754==
==5754== For counts of detected and suppressed errors, rerun with: -v
==5754== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
You can check current repository for the exact same test node. There will be a number of possibly lost bytes. This version produces the above result.
|
Could you please rerun the build for bionic? Honestly, I don't understand why it fails to produce results. In my local, it passes as follows: |
|
@ros-pull-request-builder retest this please |
|
Thanks for the patch. |
|
Thanks for the support all along the way :) |
* Resolve memory leak. Delete g_rosout_appender explicitly instead of assigning it to NULL. Follow deletion with NULL assignment. * Deregister g_rosout_appender * revert unrelated whitespace change * Update init.cpp * Increment version number. * space in comments * Merge * Revert "Increment version number." This reverts commit 795c8fd. * Add newer rosconsole dependencies * Update rosconsole dependencies in package.xml * Sync with upstream completely * Add changes * Remove deregister function, since it is already done in shutdown(). * Remove unnecessary .catkin_workspace file.
* Resolve memory leak. Delete g_rosout_appender explicitly instead of assigning it to NULL. Follow deletion with NULL assignment. * Deregister g_rosout_appender * revert unrelated whitespace change * Update init.cpp * Increment version number. * space in comments * Merge * Revert "Increment version number." This reverts commit 795c8fd. * Add newer rosconsole dependencies * Update rosconsole dependencies in package.xml * Sync with upstream completely * Add changes * Remove deregister function, since it is already done in shutdown(). * Remove unnecessary .catkin_workspace file.
* Resolve memory leak. Delete g_rosout_appender explicitly instead of assigning it to NULL. Follow deletion with NULL assignment. * Deregister g_rosout_appender * revert unrelated whitespace change * Update init.cpp * Increment version number. * space in comments * Merge * Revert "Increment version number." This reverts commit 795c8fd. * Add newer rosconsole dependencies * Update rosconsole dependencies in package.xml * Sync with upstream completely * Add changes * Remove deregister function, since it is already done in shutdown(). * Remove unnecessary .catkin_workspace file.
* Resolve memory leak. Delete g_rosout_appender explicitly instead of assigning it to NULL. Follow deletion with NULL assignment. * Deregister g_rosout_appender * revert unrelated whitespace change * Update init.cpp * Increment version number. * space in comments * Merge * Revert "Increment version number." This reverts commit 795c8fd. * Add newer rosconsole dependencies * Update rosconsole dependencies in package.xml * Sync with upstream completely * Add changes * Remove deregister function, since it is already done in shutdown(). * Remove unnecessary .catkin_workspace file.
Delete g_rosout_appender explicitly instead of assigning it to NULL.