Changed calling_in_this_thread restore mechanism in callOneCB from try-catch to RAII.#811
Changed calling_in_this_thread restore mechanism in callOneCB from try-catch to RAII.#811dirk-thomas merged 1 commit intoros:kinetic-develfrom jasonimercer:do-not-catch-user-exception-callOneCB
Conversation
|
LGTM. |
| { | ||
| // ensure that thread id gets restored, even in case of an exception | ||
| tls->calling_in_this_thread = last_calling; | ||
| throw; |
There was a problem hiding this comment.
throw without an exception argument re-throws the current exception and doesn't create a new one. Therefore I don't see why this change is necessary. Can you please provide a minimal example which demonstrates why the current code is not sufficient and "masks" the source of the original exception.
There was a problem hiding this comment.
Sure, the community link provided shows an exception that was thrown by user code but the strack trace in gdb leads to callOneCB. The exception is the same but the source of the exception is lost.
The last remark of the user in that post was "The stack trace I posted was in Debug mode. It's strange that it didn't lead me to my callback function."
I'll put together a minimal example.
There was a problem hiding this comment.
Nvm, I didn't think about it before. While it passes along the same exception it will change the reported line in e.g. gdb which is clearly undesired.
|
Thanks for the patch! Do you want this to be backported to Indigo / Jade as well? |
|
A backport isn't required for our needs. I'll let you decide if it is of value for the community and timeline accordingly. Thanks for merging this into kinetic-devel! |
|
@ros/ros_team I would like your feedback if you think this should be backported. While the patch isn't necessary it would improve the stack trace in the case of problems for users. If we agree that this change doesn't pose a risk for regressions I would move forward with the backport. |
|
I think should be safe to backport. I can't think of anyway this would affect existing code. |
|
We've been running it already in our overnight testing— it's great getting full stack traces in our crash reporter! |
…ion-callOneCB Changed calling_in_this_thread restore mechanism in callOneCB from try-catch to RAII.
This PR prevents the try-catch on callOneCB from masking the source of the original exception in a callback. We have encountering this situations here at Clearpath and the community has reported similar problems:
http://answers.ros.org/question/167386/node-crashes-on-spinonce/
@dirk-thomas @mikepurvis