Fix WebSocket disconnections, server crashes and big-file downloads - #40
Merged
Merged
Conversation
Production was returning 429 on /ws and users were dropped mid-transfer. Several independent bugs: - em-websocket 0.3.x only accepts "Connection: Upgrade" with that exact casing; nginx-proxy sends "upgrade", so every handshake failed with 500. Normalize the header before handing over to it. - Each failed handshake leaked a slot of the per-IP connection limiter (only ConnectionError was rescued), locking out the whole NAT after 50 failures. Release the slot on any failure. - The heartbeat called @ws.close, which doesn't exist on SinatraWebsocket::Connection (close_websocket does). The NoMethodError in an EM timer crashed the whole server, disconnecting everyone. Fix the call and guard every EM callback so one client can't take the server down. - app.rb's ws.onclose replaced WSClient's (single-slot callbacks), so disconnected senders were never removed and their downloads hung. - No flow control: senders pushed whole files into the socket at disk speed, the server buffered everything in memory and the heartbeat pong got stuck behind it. Add ack-based windowed streaming paced on the downloader's actual consumption, abort senders when a downloader leaves, disable thin's 30s idle timeout on downloads and send X-Accel-Buffering: no so nginx streams instead of buffering to disk. - Frontend scheduled a reconnect from both onerror and onclose, doubling sockets on every failure. Single reconnect with backoff, close stale sockets, and re-register local shares after reconnecting. - Thin mangles bracketed IPv6 Host headers into an invalid SERVER_PORT, which Rack::Lint rejects in development.
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
1 participant
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.
Problems
/wsanswered 429 Too Many Connections for everyone; Firefox couldn't connect at all.Root causes & fixes
em-websocket 0.3.8requiresConnection: Upgradewith exact casing; nginx-proxy sendsupgrade→ every handshake → HTTP 500/wsbefore handing over@ws.close— doesn't exist in this em-websocket (close_websocket);NoMethodErrorin an EM timer crashed the whole server on any late pongapp.rb'sws.onclosereplacedWSClient's (single-slot callbacks): gone senders never removed, downloads hungWSClientownsonclose, closes its streams, releases the slotstream_closewhen a downloader leaves; thin idle timeout disabled on downloads;X-Accel-Buffering: noonerrorandonclose→ sockets doubled on every failure[::1]:portHost headers →Rack::Lint500 in devSERVER_NAME/SERVER_PORTfixed up for bracketed IPv6 hostsVerification
Connection: upgrade/keep-alive, Upgrade→ 101; 60 bogus handshakes then a real client still connects; 150MB transfer to a 3MB/s downloader → md5-identical, sender queue ≤5.4MB, pong answered instantly mid-transfer; silent client closed at 60s with the server still alive; downloader abort → sender told to stop.Old cached frontends keep working with the new backend (they ignore acks). Deploying restarts the container, which also resets the leaked counters currently locking prod.