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

Better handling of idle initial connections #3941

Closed
Geal opened this issue Sep 29, 2023 · 0 comments · Fixed by #3969
Closed

Better handling of idle initial connections #3941

Geal opened this issue Sep 29, 2023 · 0 comments · Fixed by #3969
Assignees

Comments

@Geal
Copy link
Contributor

Geal commented Sep 29, 2023

If a connection to the router is opened without sending anything, it will not be closed right away by hyper's graceful shutdown feature.
In browsers like Chrome, opening a page like the health check or prometheus results in one connection created for the initial request, and another eagerly created but not used (probably in case multiple requests must be done to download assets).
If we try to close the router at this point, graceful shutdown closes the first connection, but not the second one, because it has not sent anything. The connection will close after a while, once the tab is closed in the browser or the keepalive timeout triggers.

Proposal to solve this:

In this part of the code:

stream
.set_nodelay(true)
.expect(
"this should not fail unless the socket is invalid",
);
let connection = Http::new()
.http1_keep_alive(true)
.http1_header_read_timeout(Duration::from_secs(10))
.serve_connection(stream, app);
tokio::pin!(connection);
tokio::select! {
// the connection finished first
_res = &mut connection => {
}
// the shutdown receiver was triggered first,
// so we tell the connection to do a graceful shutdown
// on the next request, then we wait for it to finish
_ = connection_shutdown.notified() => {
let c = connection.as_mut();
c.graceful_shutdown();
let _= connection.await;
}

app is an axum router. Hyper expects a tower service in the second argument of the serve_connection function.
We can make a wrapper service for app, that contains an AtomicBool shared with the task:

  • once the service's call() method receives the request, the bool is set to true
  • in the graceful shutdown code, we check for that bool. If it is false, that means we never receive the initial request, so we don't wait for the connection to shutdown, we close the task right away
  • if we have received at least one header, then hyper's read timeout code kicks in and can handle idle connections after the first initial requests
  • as an added bonus, we can add a timeout to cover the span between initial connection and receiving the entire request, using that bool
garypen added a commit that referenced this issue Oct 10, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fix #3124
Fix #3941

<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]: It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.

---------

Co-authored-by: Bryn Cooke <BrynCooke@gmail.com>
Co-authored-by: Gary Pennington <gary@apollographql.com>
This was referenced Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant