Skip to content

Ember H2 support#5657

Merged
rossabaker merged 74 commits intohttp4s:series/0.23from
ChristopherDavenport:h2Support
Jan 26, 2022
Merged

Ember H2 support#5657
rossabaker merged 74 commits intohttp4s:series/0.23from
ChristopherDavenport:h2Support

Conversation

@ChristopherDavenport
Copy link
Copy Markdown
Member

@ChristopherDavenport ChristopherDavenport commented Dec 4, 2021 •
edited by armanbilge
Loading

Http4s Core

  • Add Http Version to Request/Response

Server

  • ALPN H2
  • Http2 Prior Knowledge
  • H2c Upgrade
  • Push Promises

Client

  • ALPN H2
  • Http2 Prior Knowledge
  • h2c Upgrage - 🛑 Wont happen in this PR - Needs a different client work-in method where we can negotiate and leak a connection from h1 into h2
  • Push Promises

ScalaJS

ChristopherDavenport and others added 22 commits December 3, 2021 14:55
@ChristopherDavenport
Initial Core Port
064c5bc
@ChristopherDavenport
Add Initial Support to Builder
8d2c21d
@ChristopherDavenport
H2 Server ALPN
3c6a995
@ChristopherDavenport
Working Example with ALPN
5b94385
@ChristopherDavenport
Add Http2 Prior Knowledge
dccf80a
@ChristopherDavenport
Complete h2c mechanism
9f4bf32
@ChristopherDavenport
Simple Imports
be1c308
@ChristopherDavenport
H2 Client ALPN, and Prior Knowledge
6fc267e
@ChristopherDavenport
Working Example
5c97ac1
@ChristopherDavenport
Cross-Platform Support
51a1a8c
@ChristopherDavenport
scalafmt
ce2c8c6
@ChristopherDavenport
scalafmt sbt
3212452
@ChristopherDavenport
quickLint
690ea41
@armanbilge
Create HpackSuite
337e3ca
@armanbilge
Merge branch 'h2Support' of https://github.com/ChristopherDavenport/h…
6e58cd9 
…ttp4s into feature/ember-h2
@armanbilge
Get hpack working on JS
0f535d4
@armanbilge
Create ember-client-js-h2 example
3a82f56
@ChristopherDavenport
Merge pull request #7 from armanbilge/feature/ember-h2
947a472 
JS support
@ChristopherDavenport
Get Ahead of the change that makes applicationProtocol throw on null
7bb647f
@armanbilge @ChristopherDavenport
Close leaked resources
75508dd 
Co-authored-by: Christopher Davenport <[email protected]>
@ChristopherDavenport
Merge pull request #8 from armanbilge/feature/ember-h2
fa2d83e 
Close leaked resources
@ChristopherDavenport
Before the Abyss
0b083be
@armanbilge armanbilge mentioned this pull request Dec 5, 2021
Merged
@ChristopherDavenport
Copy link
Copy Markdown
Member Author

ChristopherDavenport commented Dec 5, 2021

I may need to hold off on h2c for client. Our current pooling mechanism is not compatible with the h2 connection model.

@armanbilge armanbilge mentioned this pull request Dec 5, 2021
Closed
rossabaker
rossabaker reviewed Jan 21, 2022
View reviewed changes
ember-core/shared/src/main/scala/org/http4s/ember/core/h2/H2Frame.scala
import cats.syntax.all._
import scodec.bits._

private[ember] sealed trait H2Frame
Copy link
Copy Markdown
Member

@rossabaker rossabaker Jan 21, 2022

Choose a reason for hiding this comment

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

Yeah, my eyes glazed over on this file. I'm trusting there are tests to give confidence there aren't errors in all these constants.

rossabaker
rossabaker reviewed Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
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.

I need a deeper understanding of the protocol before I can give a deeper review, but the two big ones are:

  1. I don't like turning off warnings, but we can leave an issue to find a way to rein that back in. I'm less averse to turning off numeric widening, particularly because I didn't notice any of the lossy bad ones.

  2. I would fix all the Throwables before those end up in people's error channels. That's a semantic change when we clean it up later.

ember-server/shared/src/main/scala/org/http4s/ember/server/EmberServerBuilder.scala
val additionalSocketOptions: List[SocketOption],
private val logger: Logger[F],
private val unixSocketConfig: Option[(UnixSockets[F], UnixSocketAddress, Boolean, Boolean)],
private val enableHttp2: Boolean,
Copy link
Copy Markdown
Member

@rossabaker rossabaker Jan 21, 2022

Choose a reason for hiding this comment

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

Is this always orthogonal to other settings? If we opt into this, we speak 1 and 2? Would there ever be a desire to speak 2 only?

Copy link
Copy Markdown
Member Author

@ChristopherDavenport ChristopherDavenport Jan 25, 2022

Choose a reason for hiding this comment

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

We could try to introduce http2 only support. I hadn't built that presently.

Copy link
Copy Markdown
Member

@rossabaker rossabaker Jan 26, 2022

Choose a reason for hiding this comment

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

I'm mostly concerned about the future state space there. Now there are two states, so all is well:

  • HTTP/1: enableHttp2 = false
  • HTTP/1 and HTTP/2: enableHttp2 = true

If there's HTTP/2 only, now what? You could introduce an enableHttp1 flag, but what happens when they're both false? Or you could preemptively make this a sum type, but you can't add to that without breaking bincompat. Or you could make all three states now, but don't support the third state yet. I don't think there's going to be a good answer here, but worst case, the server can fail to start on senseless arguments.

@mergify mergify bot added the module:core label Jan 24, 2022
armanbilge
armanbilge reviewed Jan 25, 2022
View reviewed changes
@rossabaker
Copy link
Copy Markdown
Member

rossabaker commented Jan 26, 2022

I have nearly fixed all the warnings. A @nowarn takes care of the nuisance Int => Long, which I wish never warned. (It allows evil floating point conversions, but I don't see any.) The rest are mostly unused names, but it caught some vars and what I believe is a bug.

It killed my battery before I was done. We can merge this and then fix the warnings, or I can PR to this tomorrow. Don't care which.

rossabaker
rossabaker approved these changes Jan 26, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Let's not belabor this PR with the warnings, but let's get those restored before release. I'll open a new PR.

@rossabaker rossabaker merged commit 6f4666a into http4s:series/0.23 Jan 26, 2022
@armanbilge armanbilge mentioned this pull request May 1, 2022
Closed
@kubukoz kubukoz mentioned this pull request Feb 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@armanbilge armanbilge armanbilge approved these changes

@danicheg danicheg danicheg left review comments

@rossabaker rossabaker rossabaker approved these changes

Assignees

No one assigned

Labels

module:core module:ember-client module:ember-core module:ember-server series/0.23 PRs targeting 0.23.x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ChristopherDavenport @armanbilge @bplommer @rossabaker @danicheg