Skip to content
New issue

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

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

Already on GitHub? # to your account

Add support for websocket protocol upgrade. #59

Merged

Conversation

celiocidral
Copy link
Contributor

@celiocidral celiocidral commented Oct 29, 2021

A ring handler can detect a websocket protocol upgrade request by inspecting the Connection and Upgrade headers. Then the ring handler may decide to proceed with the upgrade by returning HTTP status 101 (Switching Protocols) with the appropriate websocket upgrade response headers. Then the ring adapter automatically upgrades the HTTP request to a websocket connection. In this case the ring adapter expects the response map to have a :ws key containing the websocket handler.

The motivation here is to allow clients to connect on the same URI path for regular HTTP calls as well as websockets. E.g. GraphQL clients typically connect on the same URI path (e.g. /graphql) for running queries and making subscriptions over websocket.

Two unresolved issues in this PR:

  • websocket-upgrade-response? is case-sensitive with regards to reading the response headers. (done).
  • The protocol upgrade support needs to be replicated in proxy-async-handler (done).

I'm planning to fix those two issues in case you approve this draft. There might be other issues that escaped me that you'll probably catch. Please let me know.

More information on switching protocols at https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/101

A ring handler can detect a websocket protocol upgrade request by inspecting the `Connection` and `Upgrade` headers. Then the ring handler may decide to proceed with the upgrade by returning HTTP status 101 (Switching Protocols) with the appropriate websocket upgrade response headers. Then the ring adapter automatically upgrades the HTTP request to a websocket connection. In this case the ring adapter expects the response map to have a `:ws` key containing the websocket handler.

The motivation here is to allow clients to connect on the same URI path for regular HTTP calls as well as websockets. E.g. GraphQL clients typically connect on the same URI path (e.g. `/graphql`) for running queries and making subscriptions over websocket.

More information on switching protocols at https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/101
@celiocidral
Copy link
Contributor Author

I just realized there was already a related open issue: #41

@sunng87
Copy link
Owner

sunng87 commented Oct 30, 2021

Thank you! This is a great step to merge our websocket handler with ring handler.

After this patch being merged, I think we can make this a break change to remove legacy websocket handler APIs.

@celiocidral celiocidral marked this pull request as ready for review November 1, 2021 12:28
@celiocidral celiocidral requested a review from sunng87 November 1, 2021 12:29
@celiocidral
Copy link
Contributor Author

I admit that I felt tempted to make it compatible with the upcoming Ring 2.0 spec, but it requires listeners to implement the Listener protocol (which we would have to define first), and it also doesn't include support for ping/pong frames which is something we need in our websockets. Just wondering, do you see any need to have that compatibility in advance, before Ring 2.0 comes out?

@sunng87
Copy link
Owner

sunng87 commented Nov 1, 2021

@celiocidral We can wait for the final release of Ring 2.0 to support that. Also I am going to support both of Ring's Listener protocol and our handler map for websocket. Ping/Pong handling is important feature that I want developer to have control over it.

@sunng87 sunng87 merged commit 152d012 into sunng87:master Nov 1, 2021
@sunng87
Copy link
Owner

sunng87 commented Nov 1, 2021

Thank you @celiocidral ! I'm going to deprecate :websockets option of run-jetty and update our README soon.

@celiocidral
Copy link
Contributor Author

Sounds great, thanks!

Just a quick postmortem comment: for comparing header values, I think String.equalsIgnoreCase might perform a little bit better than lower-case.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants