Ember Web Socket Client (continued) - #7883
Open
dabrowski-adam wants to merge 127 commits into
Open
dabrowski-adam wants to merge 127 commits into
dabrowski-adam wants to merge 127 commits into
Conversation
Add validation for server opening handshake response in Ember WebSocket client
dabrowski-adam
force-pushed
the
ember-wsclient-fix
branch
from
June 30, 2026 21:56
881452a to
2bbe420
Compare
dabrowski-adam
force-pushed
the
ember-wsclient-fix
branch
from
June 30, 2026 22:35
167122c to
58c2539
Compare
dabrowski-adam
force-pushed
the
ember-wsclient-fix
branch
3 times, most recently
from
July 1, 2026 06:33
10026ff to
71d6126
Compare
When opening a WebSocket connection, the HTTP client parses the 101 Switching Protocols response off the socket. A 101 is treated as having no body, so any bytes the parser read past the response header terminator are returned via its drain. Those trailing bytes are the beginning of the WebSocket stream, which the server may coalesce into the same TCP segment as the handshake response. EmberClientBuilder stored only the raw socket in the WebSocket response attribute and discarded the drain, so the WebSocket read loop read from socket.reads directly and lost any frames the server sent immediately after the handshake. This caused receive/receiveStream to hang until the test timeout, deterministically on Scala.js and intermittently on the JVM. Capture the parser's leftover bytes for the upgrade and replay them ahead of socket.reads in the read loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dabrowski-adam
force-pushed
the
ember-wsclient-fix
branch
from
July 1, 2026 06:47
71d6126 to
30ae7fb
Compare
A 101 Switching Protocols hijacks the raw socket for the WebSocket, but EmberClientBuilder still ran postProcessResponse in the response finalizer. At teardown that returned the connection to the HTTP pool marked reusable and seeded with leftover WebSocket bytes in nextBytes, started a pre-emptive read racing the WebSocket reader, and double-evaluated the parser drain. Skip post-processing for a successful upgrade so the connection stays DontReuse (the pool closes it) and the parser's drain seeds only the WebSocket stream, never nextBytes for a reused HTTP connection. Add a regression test that coalesces the first frame with the 101 response in a single write, exercising the leftover-replay path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WSClientHighLevel.connectHighLevel documents that pongs are replied automatically, and RFC 6455 §5.5.2 requires an endpoint to answer a Ping with a Pong. The receiving endpoint may be the client, since a server is allowed to ping. EmberWSClient passed respondToPings = false with no lower layer that pongs, so high-level connections silently dropped server pings. Pass respondToPings = true so the documented behavior holds. The low-level connect surface is unaffected and still leaves ping handling to the caller. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WSFrame extends Product with Serializable, so the least upper bound of its case classes is WSFrame itself and munit's assertEquals accepts the Some(...) and List(...) literals without the : WSFrame / : WSDataFrame ascriptions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the hand-written `implicit evidence$1: Foldable[G]` parameter with a `G[_]: Foldable` context bound and eta-expand `send`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sequence the four handshake checks with *> (short-circuiting on the first Left, as mapN did) instead of a tuple mapN with a throwaway destructure. The checks are annotated with their common Either[ServerHandshakeError, _] type so the chain unifies the error type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Map over the single effect (clientHandshake) instead of lifting the pure serverHandshake result into F with pure[F] inside a for-comprehension, and use a MonadThrow context bound. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
toWebSocketFrame only needs pure and fromEither, so drop the over-constrained Concurrent bound in favor of MonadThrow and use a single F summoner instead of alternating Applicative/MonadThrow. Removes the now-unused Applicative and Concurrent imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace `if (x.isDefined) F.unit else offer` with `offer.whenA(closed.isEmpty)`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Destructure the makeCaseFull result into (response, drain) instead of referring to it positionally as responseResource._1 / ._2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the four match blocks with Either.cond for the boolean guards and Option#toRight for the Sec-WebSocket-Accept extraction. The leftmost check carries the ServerHandshakeError type so the *> chain unifies; the middle checks widen into it via covariance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dabrowski-adam
force-pushed
the
ember-wsclient-fix
branch
from
July 1, 2026 23:29
7754e18 to
01032a1
Compare
dabrowski-adam
marked this pull request as ready for review
July 2, 2026 08:54
rossabaker
requested changes
Sep 8, 2026
rossabaker
left a comment
Member
There was a problem hiding this comment.
Thanks for the submission, but I have major concerns.
-
We can't accept commits that list AI tools (such as Claude) as a co-author, as they can't consent to the license. Please rebase or amend your commits to remove the Co-authored-by: trailer and update the PR.
-
More importantly, this is a sophisticated new feature, and the maintainers are stretched thin. I spent my holiday weekend dealing with security issues, primarily in Ember and web sockets. This is right at the intersection, and history tells me it will be be laborious to harden and maintain. Who volunteers to keep this running over the long haul if we add it?
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
5 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 PR expands upon #7261. It contains the changes I made to make the tests pass on joan38/kubernetes-client#239 and also some improvements I didn't think of before that Claude Fable pointed out (which I have verified).
Closes #5227