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

Ember WebSocket on 0.22 #4803

Merged
merged 41 commits into from Jul 8, 2021
Merged

Conversation

RaasAhsan
Copy link
Member

@RaasAhsan RaasAhsan commented May 5, 2021

Supersedes #4765. Copying the old PR description over:

Introduce initial support for the WebSocket API on Ember Server. The main goal here is to comply with the existing WebSocket model; refinements are going to come through in followups. The work here is pretty straightforward to the point that I have an appetite to implement a WebSocket client as well. I think that is uncharted territory though.

Something I've been thinking about is whether we want to release this immediately or wait (maybe we release on 1.x, maybe we wait for some followups to complete). I'm not personally a huge fan of long-lived development branches, but we could merge and let the code sit on mainline without exposing a flag to enable it. FWIW, I think this PR will be fairly easy to merge forward to main.

TODO:

  • Think about how to expose websocket support
  • Cover all error scenarios and ensure proper resource cleanup
  • Address all the TODOs scattered around
  • Rely on a custom WebSocket parser rather than the existing FrameTranscoder?
  • Tests and benchmarks
  • Address old PR comments

@RaasAhsan RaasAhsan added enhancement Feature requests and improvements module:ember-server labels May 5, 2021
@RaasAhsan RaasAhsan mentioned this pull request May 5, 2021
5 tasks
@RaasAhsan RaasAhsan changed the base branch from main to series/0.22 May 5, 2021 08:04
@RaasAhsan
Copy link
Member Author

Nearly done here with the PR. Just need to consider what happens in various error scenarios and respond appropriately. And I think I'll slide in some tests

@rossabaker
Copy link
Member

So the Circe release is imminent, which was our trigger for 0.22.0. We need to decide if this is still going into 0.22, or held back for 1.0. We're probably looking at early this week. Maybe do an RC for Rho's sake.

core/src/main/scala/org/http4s/Protocol.scala Outdated Show resolved Hide resolved
core/src/main/scala/org/http4s/headers/Upgrade.scala Outdated Show resolved Hide resolved
case Some(nextBuffer) => Some(((req, resp), (nextBuffer, true)))
case None => None
}
// TODO: Should we pay this cost for every HTTP request?
Copy link
Member

Choose a reason for hiding this comment

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

In Blaze, we opt into web socket support by wrapping another stage, but it's one more thing to trip over configuring for users. I'm not sure the opt-in is worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if we could be a bit smarter here for keep-alive connections. If we know we're on one, the upgrade should never happen. I may do some performance tests later, but since this is on a 0.22 milestone I may just defer as a follow-up since opt-in is something we can add in later

* limitations under the License.
*/

package org.http4s.ember.server.internal
Copy link
Member

Choose a reason for hiding this comment

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

If there's nothing ember here, a more general internal package might be good. There's renewed talk of servlet support, and it would look a lot like this with servlet's HttpUpgradeHandler.

Although if it's all internal, we can refactor it later. Meh. It's fine where it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

There isn't much ember-specific but it does rely on fs2's Socket, which we should be able to abstract over with some effort. I've been wanting to do that anyway so we can write some tests with more depth

@rossabaker rossabaker added this to the 0.22 milestone May 23, 2021
@rossabaker
Copy link
Member

This has been targeted at 0.22, but it can slip. Tagging it so we decide.

@rossabaker
Copy link
Member

It looks like this is all additive, so we don't need to agonize over whether it makes the 0.22.0 bus. We can introduce it in a patch if it's wanted in 0.22.

@RaasAhsan
Copy link
Member Author

Going to clean up and add some tests, and should be in a good spot to merge

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

This is looking good. Anything we can't lock into 0.22.0 and 0.23.0?

@RaasAhsan
Copy link
Member Author

RaasAhsan commented Jul 7, 2021

I don't think there are any public API changes here. thought about exposing an option to enable this for performance since blaze had one but I was going to look more into that as a follow up. When are you planning to cut the 0.22.0 and 0.23.0 releases ?

also I can take care of the downstream merges

@rossabaker
Copy link
Member

I would love to cut those releases this week, but I'm on vacation, so my OSS estimates depend a lot on the local weather.

An option should be implementable without breaking bincompat, if deemed worthwhile.

@rossabaker rossabaker merged commit 5c73f8a into http4s:series/0.22 Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements module:ember-server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants