Add request task cancellation, and improve handling of graceful shutdown #379
Conversation
…oses early. In addition we check for the HTTPServer's service being cancelled or gracefully shutting down.
|
Make sure you hide whitespace on github |
|
|
||
| while true { | ||
| // set to processing unless it is cancelled then exit | ||
| guard processingRequest.exchange(.processing, ordering: .relaxed) == .idle else { break } |
There was a problem hiding this comment.
I didn't add the graceful check here, I wasn't sure if that was "correct" because we already received .head
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## 2.x.x #379 +/- ##
==========================================
- Coverage 84.51% 84.47% -0.04%
==========================================
Files 95 95
Lines 5283 5283
==========================================
- Hits 4465 4463 -2
- Misses 818 820 +2 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| // Process next request unless it is cancelled then exit | ||
| if Task.isShuttingDownGracefully || Task.isCancelled { |
There was a problem hiding this comment.
Cancellation check is moved down. I can imagine that some clients might fail if we do respond, but don't consume the body before closing the connection.
| asyncChannel.channel.close(mode: .input, promise: nil) | ||
| } | ||
|
|
||
| try await asyncChannel.channel.closeFuture.get() |
There was a problem hiding this comment.
This shouldn't be needed. executeThenClose makes sure that the channel is closed
There was a problem hiding this comment.
The reason I do this is because I want to trigger .cancelAll() if the channel closes while a request is still being handled
|
In a separate PR can you create a test that demonstrates this is an issue. If someone cancels a request would that not close the channel and as soon as the channel is closed writing to it will throw an error and the processing of the request will be cancelled. |
|
Or are you looking at the situation where generating the response takes a long time, not writing the response. |
There was a problem hiding this comment.
Removing the graceful shutdown handler means if the connection is open but waiting for http parts it will not shutdown.
You could probably do this with a middleware where you require the context to have a reference to the channel and then apply it to the routes that might require this kind of support.
Should solve a typical dilemma I've had in systems that get under heavy load: