http4s-jdk-http-client icon indicating copy to clipboard operation
http4s-jdk-http-client copied to clipboard

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

Open armanbilge opened this issue 1 year ago • 1 comments

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

https://github.com/http4s/http4s-jdk-http-client/blob/b360b86d31cf6e2617737e7edbeaca9f7a5ea027/core/src/main/scala/org/http4s/jdkhttpclient/JdkWSClient.scala#L45-L48

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 avatar Nov 27 '23 05:11 armanbilge

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

https://github.com/http4s/http4s-jdk-http-client/blob/b360b86d31cf6e2617737e7edbeaca9f7a5ea027/docs/README.md?plain=1#L178-L180

Not sure why I thought this behavior is reasonable :sweat_smile:

amesgen avatar Nov 27 '23 18:11 amesgen