Fix for #11407#11447
Merged
Merged
Conversation
…ient: * Its server loop (for accepting calls from the Hub) should block until the client has actually registered with the hub (rather than busy waiting) * More importantly, there needs to be a lock associated with whether or not the client is registered. The lock does not need to be held by SAMPWebClient.register because the _server_thread blocks until the _registered_event is set. However, SAMPWebClient.unregister does need to hold the lock to ensure it does not unregister the client when it is about to perform a pullCallbacks request to the hub.
…d now be fixed. Please unrevert again if there are still problems with this test :)
Member
|
Since this fixes a long standing bug, I think it needs a change log. The "affects dev" portion is just for reverting my hacky test patch. But let's hold off on the change log until after |
Member
Author
Ok, though the bug is still just in code that's only used in the tests. It's not code that is ever intended for use by users. It's code that's mean to emulate someone using a browser-based SAMP implementation. |
Member
|
Ah, okay. No need for change log then. Thanks! |
eteq
pushed a commit
that referenced
this pull request
Apr 2, 2021
astrofrog
pushed a commit
that referenced
this pull request
Aug 16, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts the workaround for #11407 and provides a fix that addresses the underlying bug (which can be reproduced locally under the right conditions).
The bug was a race condition whereby the
SAMPWebClient(which makespullCallbackscalls to the hub, which is how SAMP web clients perform long polling) unregisters itself from the hub, but then immediately makes apullCallbacksrequest (since the unregistration happened betweenif self._is_registeredand thepullCallbackscall here).In normal practice this would be a rare race condition, though on busy CI servers it's probably more common as there can be more delay between context switches.
This bug probably has existed for a while but was merely masked by the bug fixed by #11391.