Release the websocket promptly when a connection is disconnected#16
Merged
Conversation
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
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.
We+ CEM heeft StekkerS2 vorige week 20+ minuten geweerd met een 403 ("you already have an RM connection") terwijl wij in een reconnect-cyclus zaten. CEM-kant zag de oude TCP/WS-connectie nog als levend; pas toen onze app uiteindelijk een CLOSE 1000 verstuurde, lukte de eerstvolgende poging.
Oorzaak in deze gem:
S2::Connection#disconnectzet@stopping = trueen roept@session&.stopaan — maar tussen reconnect-iteraties is@session = nil(gezet in deensurevanconnect_and_run), dus die roep is een no-op. De running async task (en daarmee de open WebSocket binnenAsync::WebSocket::Client.connect(...) do |ws| ... end) blijft daardoor leven tot iets anders hem ontwart. CLOSE 1000 wordt pas verstuurd wanneer de block-form alsnog uitloopt, in productie minutenlang na de eigenlijkedisconnect-call.Fix:
task:is nu optioneel inS2::Connection.new. Inconnectlazy resolved naarAsync::Task.current(= de task dieconnectdaadwerkelijk draait).disconnectroept@task&.stopaan. Dat raisedAsync::Stopin de task, ontwartsleep @backoffof een blocking@ws.read, de ensures vuren, de block-form sluit de WebSocket en stuurt CLOSE 1000 — direct in plaats van na een onbepaalde tijd.Bestaande specs gaven
task: Async::Task.currentmee, wat feitelijk de OUTER spec-task was (niet de task dieconnectdraait). Met de fix zou dat de spec laten crashen; daarom is het argument weggehaald. Nieuwe spec bewijst datdisconnectalleen al de task termineert terwijl die insleep @backoffzit — red-green-bevestigd door de fix even terug te draaien.Na merge: gem-pin in
StekkerS2/Gemfile.lockbumpen, daar volgt aansluitend een PR voor.