Safely stopping `waitForSubscriptionEvent()` from another thread.

[de-vri-es]

First of all, thanks for the library. Very useful :)

When using subscriptions, it is necessary to call waitForSubscriptionEvent(), likely in a loop. However, since this can block arbitrarily long, it generally has to be put in a separate thread from a different event loop like ros::spin().

When it's time to quit the program, it becomes necessary to stop the background thread. However, from what I gathered, I don't think there is a thread safe way to do this.

Firstly, both waitForSubscriptionEvent() and endSubscription() mutate a member evaluation_conditions_. So they can't safely be called from different threads at the same time. I didn't look further, but it could be that webSocketRecieveFrame() called from waitForSubscriptionEvent() does more thread unsafe things.

Secondly, endSubscription() only seems to ask the robot controller to remove the subscription. It doesn't do anything to stop waitForSubscriptionEvent(). Should communication with the controller be broken at this point, it may take arbitrarily long for waitForSubscriptionEvent() to return. Also, I'm not sure if the robot controller closes the websocket connection at all when the subscriptions are deleted. The documentation doesn't say anything on the subject.

Note that maybe endSubscription() shouldn't interrupt a running waitForSubscriptionEvent(). But then another function to do so would be very useful to cleanly terminate an application. Doing it in a separate function allows to limit thread safety guarantees to the interrupt function only, which wouldn't necessarily have to do an HTTP request. So that might keep things easier.

[jontje]

First of all, thanks for the library. Very useful :)

You are welcome, I am happy it is used 😄

Firstly, both waitForSubscriptionEvent() and endSubscription() mutate a member evaluation_conditions_. So they can't safely be called from different threads at the same time. I didn't look further, but it could be that webSocketRecieveFrame() called from waitForSubscriptionEvent() does more thread unsafe things.

Then maybe it would be a good idea to make waitForSubscriptionEvent() allocated its own instance of EvaluationConditions.

Secondly, endSubscription() only seems to ask the robot controller to remove the subscription. It doesn't do anything to stop waitForSubscriptionEvent(). Should communication with the controller be broken at this point, it may take arbitrarily long for waitForSubscriptionEvent() to return. Also, I'm not sure if the robot controller closes the websocket connection at all when the subscriptions are deleted. The documentation doesn't say anything on the subject.

After calling endSubscription(), then the robot controller will close the current subscription and send out a WebSocket closing frame. Any active waitForSubscriptionEvent() call is looking for such closing frames and will abort. Also, if the robot controller is restarted/shutdown, then the waitForSubscriptionEvent() call will also abort.

Note that maybe endSubscription() shouldn't interrupt a running waitForSubscriptionEvent(). But then another function to do so would be very useful to cleanly terminate an application. Doing it in a separate function allows to limit thread safety guarantees to the interrupt function only, which wouldn't necessarily have to do an HTTP request. So that might keep things easier.

Yes, I can see that it would be useful to be able to close the WebSocket connection from the client side.

Thanks for your remarks!

[de-vri-es]

Thanks for your replies. I indeed went with adding a new function in #60

Then maybe it would be a good idea to make waitForSubscriptionEvent() allocated its own instance of EvaluationConditions.

I was considering making a PR to do this for every function, and removing the evaluation_conditions_ member entirely. The more local a piece of state can remain, the easier it is to reason about the behavior of code. It might also enable the compiler to do smarter optimizations. Would such a PR be appreciated?

[jontje]

I was considering making a PR to do this for every function, and removing the evaluation_conditions_ member entirely. The more local a piece of state can remain, the easier it is to reason about the behavior of code. It might also enable the compiler to do smarter optimizations. Would such a PR be appreciated?

I like your arguments, and then I guess it is also reasonable to do the same with uri_, content_, subscription_content_ and evaluation_conditions_. They don't really need to be shared.

Plus maybe adding two mutexes?

• One for protecting subscription_group_id_ in startSubscription() and endSubscription().
• The other for protecting log_ and xml_parser_ in evaluatePOCOResult(...).

Might this be something you are willing to do?