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

Implement WebSocket stream with a single send-receive pipe #3325

Merged
merged 2 commits into from Jun 11, 2020
Merged

Implement WebSocket stream with a single send-receive pipe #3325

merged 2 commits into from Jun 11, 2020

Conversation

Q69K
Copy link

@Q69K Q69K commented Apr 14, 2020

This kind of API is much more easy to use and it is kinda superset of old API - as you see I can implement old API via new, but it's very hard to do new API with an old one.
Related to #1863

@Q69K
Copy link
Author

Q69K commented Apr 14, 2020

look like you have unstable test :((

@rossabaker
Copy link
Member

rossabaker commented Apr 20, 2020

Yes, the test failure should be addressed by #3338.

@rossabaker rossabaker added the breaking Change breaks compatibility (binary or source) label Apr 20, 2020
Copy link
Member

@rossabaker rossabaker left a comment

Thanks!

I think I like this in principle, but I don't do a lot with web sockets, so we should hear from someone who does. I'd like to eventually unify server and client web socket models, so I'll summon @amesgen.

core/src/main/scala/org/http4s/websocket/WebSocket.scala Outdated Show resolved Hide resolved
@deprecated("Parameter has been renamed to `receive`", "0.18.0-M7")
def write: Pipe[F, WebSocketFrame, Unit] = receive
) extends WebSocket[F] {
override def receiveSend: Pipe[F, WebSocketFrame, WebSocketFrame] =
Copy link
Member

@rossabaker rossabaker Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we could duplicate less of the builder functions by doing this in build, and then call buildWithSinglePipe?

Copy link
Author

@Q69K Q69K Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made something of a currying with overloaded pair of build methods

)
}
}

def apply[F[_]: Applicative]: Builder[F] = new Builder[F]
def apply[F[_]: Concurrent]: Builder[F] = new Builder[F]
Copy link
Member

@rossabaker rossabaker Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the most controversial change. It's why this will be a binary-breaking change, even though F will almost always be Concurrent in practice.

I think we should either upgrade the constraint on the constructor to Concurrent, or move the Applicative constraint to buildWithSinglePipe, the way it is on build. Our builders do more of the former, though I've never convinced myself one style is superior to the other.

Copy link
Author

@Q69K Q69K Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I eliminated dependency on Concurrent by moving concurrently construction back to Http4sWSStage

@Q69K
Copy link
Author

Q69K commented Apr 20, 2020

@rossabaker thanks for the review, will take a look in the upcoming weekend

@Q69K
Copy link
Author

Q69K commented Apr 27, 2020

For the rationale of the convenience of such construction, I want to bring my one example.

A pretty simple message exchange
So a client connects to a websocket server selects a room and then receives/sends messages.

New solution:
It's very simple to build a single pipe Pipe[F, WebSocketFrame, WebSocketFrame] that is just a flatMap/mapEval over incoming messages.
I also can imagine any other actor-like definition of a websocket handler will naturally fit a single pipe.

Current solution:
And for now I have to define two pipes and somehow bind them with some kind of queue or other concurrent stuff. I really found this task too difficult for me and decided to build this PR.

P.S. A little design note
Before, it was two pipes send and receive, and they were just combined with concurrently inside of Http4sWSStage to a single pipe - this seems a decision that is delayed from implementation from now.

@amesgen
Copy link
Member

amesgen commented Apr 27, 2020

@Q69K I find your motivation very compelling, it is the reason why we also did not use the "send pipe, receive stream" model in http4s-jdk-http-client's websocket interface.

In http4s-jdk-http-client, we used a connection model, e.g. you can open a connection via WSRequest => Resource[F, WSConnection] and then interact with the WSConnection. This is very similar to the TCP interface in fs2. Both the current WS interface in http4s and your proposed model can be implemented on top of it.

I think this PR is a nice simplification for common WS interactions; I just wanted to mention this, as in the future (in particular when we move a websocket client abstraction to http4s-core), it would be nice to have a shared base abstraction for both client and server. But this would/will require some refactoring either way, so I think it is fine to introduce a new abstraction for now.

@rossabaker
Copy link
Member

rossabaker commented May 31, 2020

Needs a scalafmt.

@amesgen, do you still like the direction here? I think you're our resident WebSocket expert.

@amesgen
Copy link
Member

amesgen commented May 31, 2020

I think this is a nice goody. I think it would be a good fit for BlazeWebSocketExample.scala, can you update it to use your new WS builder? @Q69K @klpx

The implementation looks fine, and I don't have that much time right now to work on a unified WS client + server interface, so this PR should be good to go then.

@Q69K
Copy link
Author

Q69K commented Jun 3, 2020

@amesgen updated the example. Now looks much better, thanks for the suggestion ;-)

amesgen
amesgen approved these changes Jun 3, 2020
@rossabaker rossabaker merged commit 0aefab4 into http4s:master Jun 11, 2020
@Q69K Q69K deleted the websocket-with-one-pipe branch Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change breaks compatibility (binary or source)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants