Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK WebSocket is not sufficiently "low-level" to implement WSClient #996

Open
armanbilge opened this issue Nov 27, 2023 · 1 comment
Open
Labels
bug Something isn't working

Comments

@armanbilge
Copy link
Member

Currently we are offering an implementation of the "low-level" WSClient instead of the WSClientHighLevel:

/** Create a new `WSClient` backed by a JDK 11+ http client. */
def apply[F[_]](
jdkHttpClient: HttpClient
)(implicit F: Async[F]): WSClient[F] =

It's not stated explicitly, but the implication is that when using the "low-level" interface the user is responsible for handling pings and close frames. For example, only the high-level interface promises to do this and does so in the default implementation.

However, the JDK WebSocket already handles those for us.

WebSocket handles received Ping and Close messages automatically (as per the WebSocket Protocol) by replying with Pong and Close messages. If the listener receives Ping or Close messages, no mandatory actions from the listener are required.

This defies expectations and causes bugs: in the high-level connection implementation pings and close frames are handled manually. But because the JDK client is also handling these, that causes them to be handled to twice, which leads to protocol violations and I/O errors.

My best proposal to fix this is to make a breaking bump and offer only the WSClientHighLevel going forward.

h/t @yurique for reporting and investigation.

@armanbilge armanbilge added the bug Something isn't working label Nov 27, 2023
@amesgen
Copy link
Member

amesgen commented Nov 27, 2023

This is a good point, this is definitely suboptimal and should be improved. It is actually somewhat documented here:

- Responds to Ping frames with Pongs and echoes Close frames (the received Close frame is exposed
as a [`Deferred`][Deferred]). In fact, this currently also the case for the
"low-level" mode, but this will change when other websocket backends are added.

Not sure why I thought this behavior is reasonable 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants