Skip to content

Commit

Permalink
Shield HTTP request handlers from async cancellations. (#4314)
Browse files Browse the repository at this point in the history
We now spawn a new task for every HTTP request, and wait on the
JoinHandle. If Hyper drops the Future, the spawned task will keep
running. This protects the rest of the pageserver code from unexpected
async cancellations.

This creates a CancellationToken for each request and passes it to the
handler function. If the HTTP request is dropped by the client, the
CancellationToken is signaled. None of the handler functions make use
for the CancellationToken currently, but they now they could.

The CancellationToken arguments also work like documentation. When
you're looking at a function signature and you see that it takes a
CancellationToken as argument, it's a nice hint that the function might
run for a long time, and won't be async cancelled. The default
assumption in the pageserver is now that async functions are not
cancellation-safe anyway, unless explictly marked as such, but this is a
nice extra reminder.

Spawning a task for each request is OK from a performance point of view
because spawning is very cheap in Tokio, and none of our HTTP requests
are very performance critical anyway.

Fixes issue #3478
  • Loading branch information
hlinnaka authored Jun 2, 2023
1 parent ef80a90 commit 9787227
Show file tree
Hide file tree
Showing 5 changed files with 250 additions and 104 deletions.
4 changes: 1 addition & 3 deletions docs/pageserver-thread-mgmt.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ completion, or shield the rest of the code from surprise cancellations
by spawning a separate task. The code that handles incoming HTTP
requests, for example, spawns a separate task for each request,
because Hyper will drop the request-handling Future if the HTTP
connection is lost. (FIXME: our HTTP handlers do not do that
currently, but we should fix that. See [issue
3478](https://github.com/neondatabase/neon/issues/3478)).
connection is lost.


#### How to cancel, then?
Expand Down
6 changes: 6 additions & 0 deletions libs/utils/src/http/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ struct RequestId(String);
///
/// This also handles errors, logging them and converting them to an HTTP error response.
///
/// NB: If the client disconnects, Hyper will drop the Future, without polling it to
/// completion. In other words, the handler must be async cancellation safe! request_span
/// prints a warning to the log when that happens, so that you have some trace of it in
/// the log.
///
///
/// There could be other ways to implement similar functionality:
///
/// * procmacros placed on top of all handler methods
Expand Down
Loading

1 comment on commit 9787227

@github-actions
Copy link

@github-actions github-actions bot commented on 9787227 Jun 2, 2023

Choose a reason for hiding this comment

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

1071 tests run: 1024 passed, 0 failed, 47 skipped (full report)


Flaky tests (2)

Postgres 14

The comment gets automatically updated with the latest test results
9787227 at 2023-06-02T13:53:11.562Z :recycle:

Please sign in to comment.