changed error message for single threaded spinner #1164
Conversation
| @@ -130,7 +130,7 @@ void SingleThreadedSpinner::spin(CallbackQueue* queue) | |||
|
|
|||
| 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
jgueldenstein
force-pushed
the
multithreaded-spinner-comment
branch
from
9f35bcf
to
e04469a
Compare
|
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. |
| @@ -113,8 +113,7 @@ struct SpinnerMonitor | |||
|
|
|||
| SpinnerMonitor spinner_monitor; | |||
| 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?