Repository navigation
bare websockets - #6932
Open
benedikt-bartscher wants to merge 87 commits into
Open
bare websockets#6932benedikt-bartscher wants to merge 87 commits into
benedikt-bartscher wants to merge 87 commits into
Conversation
Merging this PR will not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| 🆕 | test_transport_inbound_socketio |
N/A | 17.5 ms | N/A |
| 🆕 | test_transport_inbound_websocket |
N/A | 10.2 ms | N/A |
| 🆕 |
test_transport_outbound_socketio |
N/A | 34.4 ms | N/A |
| 🆕 | test_transport_outbound_websocket |
N/A | 17 ms | N/A |
Comparing benedikt-bartscher:make-sio-optional (62e0664) with main (4be8b4d)
Contributor
 [High impact] The default event connection changes for stateful Reflex apps. The PR still needs to preserve custom Findings
SummaryReplaces the default Socket.IO connection with a plain WebSocket transport.
Previous review context:
Reviews (75) · Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
benedikt-bartscher
marked this pull request as ready for review
August 22, 2026 22:09
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
benedikt-bartscher
marked this pull request as draft
August 22, 2026 22:27
…ace, awaited disconnect cleanup; revert server-wide ws_max_size
benedikt-bartscher
marked this pull request as ready for review
August 22, 2026 22:46
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… message shape, log protocol closes; add transport benchmarks
…cut client watchdog churn
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…icorn] extra with gunicorn
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`connect`, `disconnect` and `error` name the client handle's own lifecycle, and only the server's send path refused them: a client could emit one, and the transport handed it to on_message as an application message. A handler that relays what it receives would then answer it with a send the channel API refuses. `channel.emit` now throws on a reserved name and the transport answers an inbound one with a `reserved_event` error instead of dispatching it.
handle_connect returns without linking a SID when the query string carries no token, and logs it at debug because "the transport closes the session" -- which the plain websocket transport does and the Socket.IO one did not, leaving an unauthenticated connection open for its ping interval to keep alive. on_connect now refuses it, undoing its own connection accounting and scope first, since Socket.IO runs no disconnect handler for a refused connect.
test_uvicorn_worker_carries_the_socket_policy dropped the worker module from sys.modules and never put it back, so the rest of the session inherited a module whose CONFIG_KWARGS were baked from the test's environment. It now restores the cache it found. test_embedded_server_retries_taken_port polled for a full 15s and then failed on a bare assert whenever the rebind did not happen. It stops polling once the server thread is gone and names what went wrong.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 40 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
socket.io-client is now imported only when the Socket.IO transport is configured, so the playground export splits it into a chunk of its own: 18 chunks instead of 17, with a 10 KB smaller initial bundle.
benedikt-bartscher
marked this pull request as draft
October 8, 2026 20:12
Transport: each connection queues its frames for a writer task of its own, so a client that stops reading no longer blocks the coroutines emitting to it, and is dropped past a 16 MiB backlog. Closing goes through the session's receive loop, which granian requires to complete a close. Frames are compact JSON, escape unpaired surrogates and size attachments in bytes; a huge integer literal is a malformed frame. The event route is served with and without a trailing slash, which the client now dials, Socket.IO clients are refused before accept, and emit() without a recipient broadcasts. Client: handlers run over a copy and cannot abort the transport's own bookkeeping, the ping watchdog reports a dead link at once, a channel connects once, a throwing WebSocket constructor becomes a connect error, channel names and attachments are validated, an oversized event is dropped instead of costing the socket, and a socket that cannot be created is reported as a connection error rather than a rejection that reconnects without end. Channels reserve every underscore-prefixed name in both directions and validate fan-out with nobody listening; registering one where channels cannot run warns and skips it. A custom sio without a transport setting falls back to Socket.IO with a deprecation, and reflex.app re-exports the Socket.IO classes lazily. A plain-string cors_allowed_origins is read as a list. Auto-detection picks granian when uvicorn has no websocket library, and granian caps the message size at what it receives. AppHarness owns its port from construction, reports readiness itself, fails fast on a busy requested port or a failed lifespan and logs its errors. The benchmark clients detect the transport the backend speaks. Docs cover the transport upgrade, proxy paths and channel task pattern.
benedikt-bartscher
marked this pull request as ready for review
October 8, 2026 22:40
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 62 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
TRANSPORT in env.json is a name socket.io-client accepts again, as on main, and a SOCKETIO flag picks the protocol: third-party code hands TRANSPORT to socket.io-client to share the app's Socket.IO connection. The docs app runs on transport="socketio" with reflex[socketio], which xy 0.0.7's charts need until xy rides a channel. A boot event in a Socket.IO CONNECT packet bypasses on_event, so one that fails to deserialize refuses the connect instead of disconnecting a session not yet acknowledged. Header values that are not UTF-8 decode as Latin-1 rather than failing every event on the connection. Idle benchmark clients poll within the ping timeout. reflex-base's news scopes the deflate setting to uvicorn and lists the default transport change as breaking.
Port upstream's EventNamespace changes to the moved namespace modules: the connect-time state schema check now runs in BaseEventNamespace.handle_connect for both transports (connect_boot_event becomes connect_auth, which also returns the frontend schema), and the unknown-substate client_error report uses upstream's wording. Run client_state_updates.mjs over both the plain WebSocket and Socket.IO transports; the queue test runtime now serves dynamic imports.
Decode ASGI headers with plain str decodes and fall back to per-header Latin-1 decoding only when some header is not UTF-8, which is faster than upstream's decoding. Replace the Socket.IO namespace's _dispatch_event coroutine with a sync scope lookup, removing a coroutine layer per event, and drop the router_data type check that its normalization already covers.
A token claimed by another instance drops this socket's token link but not its connection, so channel sends (rooms, send_to_token, session.send) kept reaching it until it sent another frame. Deliver channel messages only to linked sessions and close a stale connection, whose teardown forgets its channel sessions. The client flushed its send queue on handshake without checking the limit that handshake advertises, so a frame queued before the first handshake or while a socket was closing could make the backend drop the connection. Check each queued frame and report a dropped one like emit does, or as message_too_large on its channel.
The benchmark socket client builds on reflex-bench's wire codec, and two unit tests that also run on Windows imported it. Importing any part of reflex-bench makes coverage count the whole Linux-only package, whose own tests are ignored off Linux, which pulled Windows coverage under the threshold. Run those two tests on Linux only and import the client inside them.
Both transports now open sessions through BaseEventNamespace: one connect-and-boot sequence and one handler failure policy. A failed boot event ends the session on either transport (the plain WebSocket closes with 1011), so the client retries rather than waiting for a hydrate that never comes. Undeserializable input ends a session quietly, and other handler failures are logged within the error budget; Socket.IO's on_event, on_ping and on_client_error no longer raise into python-socketio. Socket.IO packets are serialized like plain frames, so an unpaired surrogate is escaped instead of breaking the send. Also: one fan-out body for channel room and token sends, a socket policy read once per connection for both transports, a supports_channels flag instead of re-reading the config, the embedded test server capped like the Granian launchers, CORS strings parsed as the environment variable is, and an unreachable uvicorn warning removed. The Socket.IO namespace tests move from test_app.py to test_socketio_namespace.py, and the namespace tests share fixtures and a serve() helper.
websocket.js settles a closed connection in one place, flushes the channel and transport queues through one helper, and checks and reports oversized frames through one size check and one message. The single-caller emitChannel, which bypassed the channel's guards, is gone, and the generic JSON helpers move to json.js. state.js shares its warm-socket, socket disposal and connect-error helpers and drops an unreachable check. The client-only tests that ran websocket.js through the Python harness move to websocket_js.test.mjs, and the channels integration test now queues its frame on the transport, as its name says.
The benchmark socket client detects a backend's protocol once instead of per connection, so Socket.IO reconnect timings no longer include a failed WebSocket upgrade per client, and codecs carry their bare connect and ping frames instead of callers branching on the protocol. reflex-bench shares its process spawn, stop and report handling, its worker wrapper and small session helpers, and drift tests pin the protocol constants it copies from Reflex. The bench tests share their fixtures through factories.py.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 31 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
The echo server announced its generator's 16 MiB frame limit as its message size, while a default Reflex app (and its own Socket.IO greeting) announces 1 MB; the drift test only compared keys. It now announces Reflex's default on both protocols, and the test compares the whole handshake against a default-configured app. detect_protocol always waited up to 30 seconds to open its websocket, so the benchmark socket client's first connection to a hung backend ignored the caller's timeout. Detection now takes the timeout, and the client passes its own through.
Port upstream's repeated-header combining (reflex-dev#7588) from the removed reflex/app.py helper to _decode_asgi_headers in reflex/event_namespace.py, which both transports share, keeping the branch's Latin-1 fallback. Its tests move from tests/units/test_app.py to tests/units/test_event_namespace.py.
Comment on lines
+116
to
+118
| if linked and ( | ||
| boot_event is None | ||
| or await self._handle_boot_event(sid, boot_event, self._scope(sid)) is None |
Contributor
There was a problem hiding this comment.
Under transport="socketio", the boot event now reaches handle_event directly instead of self.on_event. Apps that replace EventNamespace and override on_event still run their custom code for later events, but skip it during initial hydration and reconnect hydration. Any checks or other work in that override are skipped for those requests.
Keep boot events going through the overridden on_event, with the shared error policy around that call.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Granian sets TCP_NODELAY on listeners it binds itself, but the harness binds its own socket, so with the Redis state manager each update stalled ~40 ms on delayed ACKs and test_client_side_state raced it. Also poll the client storage reads that follow an event or navigation.
Port upstream's reconnect back-off (reflex-dev#7605) to the transport abstraction: retryConnect keeps the branch's addConnectError helper and caps the attempt count at MAX_CONNECT_ERRORS. Socket.IO reports a transport closed before the namespace connected as a disconnect, which now retries with back-off; the plain WebSocket reports it as connect_error, so it gets the same back-off. The state.js back-off tests run over both transports, with the fake socket reporting a close before connecting the way each transport does, and read connect errors through upstream's lazily applied updaters.
This branch has not been deployed
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
3 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.
saves ~11.6 KB gz / ~41 KB raw frontend size (a small js chunk replaces whole socketio)
dropped python deps: wsproto, simple-websocket, python-socketio, python-engineio, bidict
new benchmarks in test_event_transport.py show about 2x performance increase compared to socketio
also migrates AppHarness to granian for consistency. otherwise downstream tests using AppHarness still need uvicorn and wsproto/websockets