pageserver management API doesn't poll request handlers to completion if client hangs up [P:1] [S:1] #3478
Comments
|
Hmm, yeah, the whole thing doesnt look right to me. This is definitely a problem. There are several problems with detach. Initially I thought about it in a way that it shouldnt block for a long time, and should be resumable. I e when it fails on something it is safe to retry the request and it will retry the thing that failed, no the operation from the beginning. So it may make sense to make it cancel safe. Detach also should work correctly through power cycle. (This should already be solved with mark file) From the other side we can go to completely async model, let detach return 202, and let the client poll it till the completion. It will make error reporting harder. Because we need to keep failed operations in memory to give them back during poll phase. Client will have to handle other states, e g if pageserver was restarted when detach was in progress. |
problame
changed the title
|
Thoughts: See how this relates to changing http framework, think about using spawn for each connection handler. Beware of potential concurrency issues, e g when retry happens when previous call didnt finish |
|
The topic is wrong, it's a feature not a bug that this happens, because there's no way to do the other way around if you'd have different defaults. I thought we had already spawned a task for detach for example, but we don't, this is causing some issues in prod: https://neondb.slack.com/archives/C03H1K0PGKH/p1680849458972369 -- at least for one tenant upload task shutdown took so long. |
If I got it correctly, then if you spawn a new thread and respond with 200 OK to the caller. There is still a chance that pageserver will restart until spawned thread finishes its job, but the caller (control-plane) won't retry call in this case anymore as it got OK already Another point to consider: in the handler only persistently set some flag that detach requested and process in in the background. Then caller will need to re-check that tenant was actually fully detached |
Obviously it should return accepted or something like that so it would be polled again. The original detach idea is that it should be cancel safe, i e the retry request should be able to continue half finished operation. |
|
We can run the detach in a task_mgr task, yet still make the request blocking, e.g., by communicating the detach result to the request handler using a We could also do this systematically in the This doesn't solve the underlying issue of detach being not cancel & crash-safe. In today's pageserver meeting we decided that @LizardWizzard scope out what it takes to make detach cancel & crash-safe. Depending on what comes out of that, we should act on my proposal in the previous paragraph. In general, I think our code is not written with cancel-by-drop-safety in mind, so, the default should be that request handlers are run to completion in a task, even if the client hangs up. |
|
Oops, it seems I've failed to post a reply in between now twice most likely. I understood the objected topic as "rust is at fault with cancellable futures" from an earlier slack discussion. My #4159 PR is very much related to this topic. I guess in the past comment which I didn't post or posted it elsewhere I was thinking of we have two choices:
With #4159's approach, we would get cancellation free futures by default, which we could modify to be cancellable. Of course we should still handle crash safety, or at least consider it. I am thinking hard to come up with a good reason why we would go through of different crash safety matters even, because we could just have a persistent log for all of the accepted operations, and upon recovery apply the changes locally, reducing the crash-safety surface to just the log. I don't mean that the log would ever be used to rollback operations, just to continue to finish the started ones. |
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
If the timeline is already being deleted, return an error. We used to notice the duplicate request and error out in persist_index_part_with_deleted_flag(), but it's better to detect it earlier. Add an explicit flag for the deletion. Note: This doesn't do anything about the async cancellation problem (github issue #3478): if the original HTTP request dropped, because the client disconnected, the timeline deletion stops half-way through the operation. That needs to be fixed, too, but that's a separate story.
If the timeline is already being deleted, return an error. We used to notice the duplicate request and error out in persist_index_part_with_deleted_flag(), but it's better to detect it earlier. Add an explicit flag for the deletion. Note: This doesn't do anything about the async cancellation problem (github issue #3478): if the original HTTP request dropped, because the client disconnected, the timeline deletion stops half-way through the operation. That needs to be fixed, too, but that's a separate story.
If the timeline is already being deleted, return an error. We used to notice the duplicate request and error out in persist_index_part_with_deleted_flag(), but it's better to detect it earlier. Add an explicit lock for the deletion. Note: This doesn't do anything about the async cancellation problem (github issue #3478): if the original HTTP request dropped, because the client disconnected, the timeline deletion stops half-way through the operation. That needs to be fixed, too, but that's a separate story. (This is a simpler replacement for PR #4194. I'm also working on the cancellation shielding, see PR #4314.)
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
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
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
|
This was fixed in PR #4314. |
Steps to reproduce
tokio::time::sleep().awaitfor 10 seconds, and log something before and after.curl.curlExpected result
Log output:
Actual result
Environment
All
Why Is This A Problem?
Some management API handlers are not cancel-safe. In fact, I suspect most of them aren't.
One example is
/detach.If the client hangs up after we've transitioned the tenant to
Stopping, but beforemgr::detach_tenantreturns, the following happens:StoppingstateStopping(rightly so)Solution
Spawn all management API tasks as
task_mgrtask so that they run to completion, even if the webserver stops polling the futures.The implementation should create some kind of middleware to do this, and that middleware should also pass in a
RequestContextto the task. OnceRequestContextlearns cancellation, the middleware should cancel theRequestContextwhen the request handling future (not the task that is spawned!) is dropped.If this issue is worked on before
RequestContextsupports cancellation, use aCancellationTokeninstead.Consider reducing control-plane timeout once issue is fixed: https://github.com/neondatabase/cloud/commit/ff390331f9bdb80d7120d30f8d6d1ea4ee588f37. Maybe some other tweaks will be needed too
The text was updated successfully, but these errors were encountered: