withAsyncTaskCleanupHandler calls its cleanup handler twice if body throws #81
Conversation
jakepetroules
force-pushed
the
eng/PR-withAsyncTaskCleanupHandler
branch
from
b443d52 to
f9c5c22
Compare
…hrows If the body closure given to withAsyncTaskCleanupHandler throws, its cleanup handler is called twice: once in the catch block where body is called, and then again in the task group task once the Task.sleep throws due to cancellation, which swallows the error and then continues to call the handler as well. That results in the teardown sequence being invoked twice, as well as the teardown sequence being invoked for non-error cases. This patch ensures the cleanup handler is invoked in failure cases only, and only once. Closes #80
jakepetroules
force-pushed
the
eng/PR-withAsyncTaskCleanupHandler
branch
from
f9c5c22 to
326b01c
Compare
| return result | ||
| } catch { | ||
| await handler() | ||
| runCancellationHandlerPromise.fulfill(with: true) | ||
| throw error |
There was a problem hiding this comment.
It seems like the issue is simply we don't have to call handler() here and let the one above run. Why do we need Promise?
On a related note, we are really trying very very hard to move away from LockedState like types like this at least on Darwin (this is why we decided to move .standardOutput and .standardError out from the Execution type, so we don't have to hold a LockedState).
There was a problem hiding this comment.
I cannot see any other way to ensure the cancellation handler runs only once and only on cancellation or failure. The group.addTask is ALWAYS going to run to completion no matter what happens, but we specifically don't want to run the teardown sequence if no cancellation was invoked or error was thrown. Otherwise you may incorrectly attempt to terminate a still-running process after the body closure runs.
If the body closure given to withAsyncTaskCleanupHandler throws, its cleanup handler is called twice: once in the catch block where body is called, and then again in the task group task once the Task.sleep throws due to cancellation, which swallows the error and then continues to call the handler as well.
This results in the teardown sequence being invoked twice.
Closes #80