Add thread-safe way to interrupt waitForSubscriptionEvent().

[de-vri-es]

This is an attempt to solve #59. It closes the socket in order to interrupt p_websocket_->receiveFrame(), and in turn waitForSubscriptionEvent().

A separate mutex is now used for setting p_websocket_ and for using it. I believe this way there is no unnecessary blocking, but also no unsafe invalidation of p_websocket_.

From my understanding, closing the socket before a full frame has been received will make receiveFrame() throw a WebSocketException, which seems fine to me.

[de-vri-es]

Note: earliest I can test this is friday. I won't have access to a controller before then.

[gavanderhoorn]

Thanks for the PR 👍 . Unfortunately reviewing and merging it is going to take some time, as both me and @jontje have a very busy few weeks ahead of us.

[de-vri-es]

Understood. Testing has shown that close() doesn't interrupt a running recvmsg(). on Linux. Plus, it could introduce a race with the fd number being reused before receiveFrame() continues, unless poco properly handles that internally.

However, shutdown() does interrupt the call and doesn't release the fd. Updated the PR to use shutdown() instead.

[de-vri-es]

Polite bump. Would a review be possible?

By now I have tested this (manually) against a real IRC5 compact. It worked as intended during those tests.

[jontje]

Thanks for the PR @de-vri-es, it's much appreciated 👍

My calendar has finally calmed down a bit, and I intend to review this and your other PRs this week.

In the meanwhile, could you make sure your PRs are up-to-date with the base branch?

[de-vri-es]

Good to hear! I updated the branches. Note that they probably have merge conflicts with each-other (because of the commit in #58 that removed trailing whitespace). I can fix them when they appear :)

[jontje]

I have tested the implementation and it seems to be working as it should, and I have also added a few suggestions to the code. I.e.:

• Clarifications to some of the descriptions.
• Name change of closeSubscription to forceCloseSubscriotion.
• Curly brackets on new lines, to be consistent with the rest of the code base.

Please let me know if you dislike any of the changes.

[jontje]

Suggested change

	*
	* Preferred way to close the subscription is to request the robot controller to end it via endSubscription().
	*

Just as a note that it is "nicer" for the robot controller to use endSubscription() when possible.

[de-vri-es]

Thanks for the review. I'll try to get round to processing the feedback on Friday.

[de-vri-es]

I think I processed the feedback. I added a slightly longer explanation that also includes when it is useful to call the new forceCloseSubscription() function.

For now, I believe endSubscription() still can't safely be called while waitForSubscriptionEvent() is running, but I'll see if that can be fixed with a follow-up PR (as also being discussed in #59). Until then the comment suggesting to call endSubscription() instead of forceCloseSubscription() is a bit misleading.

Either way, this function will still be useful if endSubscription() and waitForSubscriptionEvent() are thread safe, for the case the controller isn't responding :)

[jontje]

Thanks @de-vri-es 👍

For now, I believe endSubscription() still can't safely be called while waitForSubscriptionEvent() is running, but I'll see if that can be fixed with a follow-up PR (as also being discussed in #59). Until then the comment suggesting to call endSubscription() instead of forceCloseSubscription() is a bit misleading.

Yes, I understand what you are getting at. Currently, endSubscription() can only safely be called in-between calls to waitForSubscriptionEvent(), which isn’t very flexible.

Either way, this function will still be useful if endSubscription() and waitForSubscriptionEvent() are thread safe, for the case the controller isn't responding :)

That is true 😄