Remove destructor logging #424
Conversation
lia-viam
requested review from
njooma and
stuqdog
and removed request for
a team
| @@ -150,6 +150,8 @@ BOOST_LOG_ATTRIBUTE_KEYWORD_TYPE(attr_time, | |||
| /// @ingroup Log | |||
| /// | |||
| /// Use this macro to generate log messages pertaining to the SDK at large. | |||
| /// @warning Using this macro outside of the lifetime of LogManager is UB and for this reason | |||
| /// logging in destructors should be done with caution or avoided entirely. | |||
There was a problem hiding this comment.
So, if the rule is that an Instance must outlive ModuleService or RobotClient, how can this situation arise?
There was a problem hiding this comment.
I just encountered this when trying to run example_dial_api_key to do direct dialing, which was failing locally for me. After failing to list the resources, thread::join in close was throwing a resource_deadlock_would_occur (i.e., attempted to join current thread) and then logging that exception in ~RobotClient was causing a null pointer deref in Boost.Log, at which point a print statement confirmed that instance had been destructed before the dtor was called.
Now as for how this happens I'm not so sure honestly. I would expect C++ objects to be destroyed in the reverse order that they're created, which means Instance should be first in last out, but it seemed like that's not what was happening
So this came up while working on another PR, but it turns out logging in destructors was causing issues after all. In particular the
RobotClientdestructor may run afterInstance(and henceLogManager) have already been destroyed. I encountered this when joining threads inRobotClient::closewas throwing an exception due to joining an unjoinable thread, so that will probably need a separate hotfix, but we should merge this one soon.