Skip to content

h3/quic: add experimental option to the Android/JVM EngineBuilder#2163

Merged
goaway merged 14 commits intomainfrom
aw-ms/h3-config
Apr 21, 2022
Merged

h3/quic: add experimental option to the Android/JVM EngineBuilder#2163
goaway merged 14 commits intomainfrom
aw-ms/h3-config

Conversation

@goaway
Copy link
Copy Markdown
Contributor

@goaway goaway commented Apr 18, 2022 •
edited
Loading

Description: Adds experimental support for negotiating H3/QUIC connections. To enable this, enableHttp3 must be set to true on the EngineBuilder AND the header "x-envoy-mobile-upstream-protocol" must be set to "http3" on the request. If the upstream does not support H3, the standard ALPN over TCP configuration will still be used by default.
Risk Level: Moderate
Testing: Updated Unit Coverage

Co-authored-by: Alyssa Wilk alyssar@chromium.org
Signed-off-by: Mike Schore mike.schore@gmail.com

alyssawilk and others added 6 commits April 12, 2022 08:51
@alyssawilk
http3 config
ea061ad 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
ui
7016f54 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Merge branch 'main' into config
19e91dc 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
cache
a42f840 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@goaway
Merge branch 'main' into aw-ms/h3-config
97620d2 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
wip wiring h3 configuration to builder
a079923 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway marked this pull request as draft April 18, 2022 18:24
goaway added 2 commits April 20, 2022 20:27
@goaway
wrap up config changes
2b79839 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
format
70e58ba 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway changed the title [wip] h3 config h3/quic: add experimental option to the Android/JVM EngineBuilder Apr 20, 2022
goaway added 2 commits April 20, 2022 20:40
@goaway
codespell
470ba5b 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
codespell again
8dd2a42 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway mentioned this pull request Apr 20, 2022
Closed
@goaway goaway requested a review from RyanTheOptimist April 20, 2022 12:49
@goaway goaway marked this pull request as ready for review April 20, 2022 12:50
@goaway
fix cronet test
5be5cb4 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
Copy link
Copy Markdown
Contributor Author

goaway commented Apr 20, 2022

The setup should be functional and complete. There's a handful of failing tests that I'll fix up tomorrow, and H3 still actually needs to be tested to verify it works when enabled.

RyanTheOptimist
RyanTheOptimist approved these changes Apr 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for doing this. One question, but LGTM. (Well, once the tests are passing of course :>)

library/common/http/client.cc
cluster = H2Cluster;
// NOTE: This cluster will attempt to negotiate H3, but defaults to ALPN over TCP.
} else if (value == "http3") {
cluster = H3Cluster;
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Apr 20, 2022

Choose a reason for hiding this comment

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

Do we plan to eliminate the need to set http/3 explicitly on a per-request basis, eventually?

Copy link
Copy Markdown
Contributor Author

@goaway goaway Apr 20, 2022

Choose a reason for hiding this comment

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

Yes, but right now the configuration required for that is complicated by legacy handling of what essentially should be considered "routing" here in the Http::Client. Once we move fully to using the route table in configuration, it will be much easier to set that up.

I went with this approach both to simplify and limit the scope of this PR, and because I figured at any rate it provides an extra safety guard around a currently "experimental" feature (at least in the sense that we haven't extensively tested it in this library yet).

goaway added 3 commits April 21, 2022 17:10
@goaway
Merge branch 'main' into aw-ms/h3-config
b27b20d 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
fix tests
3d301e3 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
add test
3b949cc 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway requested a review from RyanTheOptimist April 21, 2022 13:52
@goaway goaway enabled auto-merge (squash) April 21, 2022 17:38
@goaway goaway disabled auto-merge April 21, 2022 17:39
@goaway goaway merged commit 256fe5f into main Apr 21, 2022
@goaway goaway deleted the aw-ms/h3-config branch April 21, 2022 17:39
@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Apr 22, 2022

This is causing hangs/crashes in our Lyft apps so I'm reverting this while I continue to investigate.

Here's the crash backtrace I'm seeing on iOS (not very helpful unfortunately):

* thread #11, stop reason = signal SIGABRT
  * frame #0: 0x000000013a02300e libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00000001399a61ff libsystem_pthread.dylib`pthread_kill + 263
    frame #2: 0x0000000138905684 libsystem_c.dylib`abort + 123
    frame #3: 0x00000001024793af Lyft`___lldb_unnamed_symbol4253$$Lyft + 3231
    frame #4: 0x000000010247e0c7 Lyft`___lldb_unnamed_symbol4339$$Lyft + 130
    frame #5: 0x00000001399a64e1 libsystem_pthread.dylib`_pthread_start + 125
    frame #6: 0x00000001399a1f6b libsystem_pthread.dylib`thread_start + 15

jpsim added a commit that referenced this pull request Apr 22, 2022
@jpsim
Revert "h3/quic: add experimental option to the Android/JVM EngineBui…
21b02f3 
…lder (#2163)"

This reverts commit 256fe5f.
jpsim added a commit that referenced this pull request Apr 22, 2022
@jpsim
Revert "h3/quic: add experimental option to the Android/JVM EngineBui…
910ab00 
…lder (#2163)"

This reverts commit 256fe5f.

Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim mentioned this pull request Apr 22, 2022
Closed
jpsim added a commit that referenced this pull request Apr 22, 2022
@jpsim
Revert "h3/quic: add experimental option to the Android/JVM EngineBui…
b08883b 
…lder (#2163)" (#2186)

This reverts commit 256fe5f.

Signed-off-by: JP Simard <jp@jpsim.com>
goaway added a commit that referenced this pull request Apr 22, 2022
@goaway
Revert "Revert "h3/quic: add experimental option to the Android/JVM E…
f66b358 
…ngineBuilder (#2163)" (#2186)"

This reverts commit b08883b.

Signed-off-by: Mike Schore <mike.schore@gmail.com>
goaway added a commit that referenced this pull request Apr 22, 2022
@goaway
Revert "Revert "h3/quic: add experimental option to the Android/JVM E…
2b919f4 
…ngineBuilder (#2163)" (#2186)" (#2190)

Description: This reverts commit b08883b. We were able to confirm the crash we saw with our integration was specific to our integration and not to the public implementation.
Risk Level: Low
Testing: Manual & CI

Signed-off-by: Mike Schore <mike.schore@gmail.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Apr 25, 2022

Awesome! So exciting to see this land. Is there somewhere I can doc it up after the fact, or do we think that we're switching to removing header based routing soon enough it's fine as is?

@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Apr 25, 2022

oh less awesome to see this reverted, heh missed that. maybe can get gocs when we re-land?

@goaway
Copy link
Copy Markdown
Contributor Author

goaway commented Apr 25, 2022

@alyssawilk The revert was reverted, this is on main. :)

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Apr 25, 2022

Re-landed in #2190 btw, we determined that the issues we were seeing in Lyft integrations were caused downstream.

@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Apr 25, 2022

haha, lost track of the reverts. back to excited then :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@goaway @jpsim @alyssawilk @RyanTheOptimist