changed error message for single threaded spinner #1164
changed error message for single threaded spinner #1164dirk-thomas merged 3 commits intoros:lunar-develfrom
Conversation
| if (!spinner_monitor.add(queue, true)) | ||
| { | ||
| ROS_FATAL_STREAM("SingleThreadedSpinner: " << DEFAULT_ERROR_MESSAGE); | ||
| ROS_FATAL_STREAM("SingleThreadedSpinner: " << DEFAULT_ERROR_MESSAGE << " You might want to use a MultiThreadedSpinner instead."); |
There was a problem hiding this comment.
Looking at this critically, I think we probably want to go a bit further and update the language elsewhere. In particular, the second sentence of DEFAULT_ERROR_MESSAGE doesn't seem to make much sense anymore:
const std::string DEFAULT_ERROR_MESSAGE =
"Attempt to spin a callback queue from two spinners, one of them being single-threaded. "
"This will probably result in callbacks being executed out-of-order.";
Since we are throwing a std::runtime_error directly afterwards (and thus, nothing will be happening, in-order or out-of-order). Further, it seems like we should probably use the same string both in the ROS_FATAL_STREAM and as the string we use during the throw (though this string would be different depend on whether this is SingleThreadedSpinner::spin or AsyncSpinnerImpl::start. Would you mind cleaning up those while we are doing this? Thanks.
…the same and not imply that spinning was successful when mixing single threaded spinner with multi/async spinners
9f35bcf to
e04469a
Compare
dirk-thomas
left a comment
There was a problem hiding this comment.
The current patch doesn't compile.
Please make sure that patches build locally for you and pass the tests before creating PRs.
|
Hey dirk. Sorry I pushed @jgueldenstein to reopen this request right away.
He will clean it up shortly. Sorry for the premature ping due to the re-open.
|
…ded spinner alongside a multithreaded one
|
Ping @dirk-thomas @clalancette @jgueldenstein fixed the problems with this request and CI passed last week. |
| const std::string DEFAULT_ERROR_MESSAGE = | ||
| "Attempt to spin a callback queue from two spinners, one of them being single-threaded. " | ||
| "This will probably result in callbacks being executed out-of-order."; | ||
| "Attempt to spin a callback queue from two spinners, one of them being single-threaded. "; |
There was a problem hiding this comment.
Please remove the trailing space between the dot and the closing quote.
|
Thank you for the patch and for iterating on it. |
* not imply that spinning was successful when mixing single threaded spinner with multi/async spinners * updated error messages to reflect behavior when calling a singlethreaded spinner alongside a multithreaded one
added hint to use MultiThreadedSpinner when starting a single threaded spinner failed because it was on the same queue as a previously started AsyncSpinner.
This behavior was tolerated in kinetic but recommended against.
We would also like to add a similar message to the kinetic-devel branch.
Would you support this too?