-
Notifications
You must be signed in to change notification settings - Fork 48
Made TemporalWorker.ExecuteAsync() keep working even after Dispose().
#503
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
Made TemporalWorker.ExecuteAsync() keep working even after Dispose().
#503
Conversation
cretz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nothing blocking (comments are mostly unimportant)
| protected virtual void Dispose(bool disposing) | ||
| { | ||
| if (disposing) | ||
| var disposer = Interlocked.Exchange(ref this.disposer, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not do this unless disposing is true? I guess it doesn't matter much, but it is a bit confusing to read
| // Take ownership of the disposer to make sure the workers aren't disposed before completion of this function | ||
| using var disposer = Interlocked.Exchange(ref this.disposer, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I have confirmed that even in this situation:
public Task StartWorkerAsync()
{
using var worker = new TemporalWorker(someOptions);
// Note this does not take the disposer because code is not run
// inside of this because it is "async" so it waits for first "await"
return worker.ExecuteAsync(someToken);
// worker is disposed here
}
var task = StartWorkerAsync();
await task;The disposer is properly transfered before the ExecuteAsync call is returned. But yes it will be important for future SDK devs to make sure no yieldable await points are ever added before this line is called. Technically we can help with this by doing this in the outer ExecuteAsync calls and removing the async keyword from them, but I am fine with this too.
| return await worker.ExecuteAsync(() => action(worker)); | ||
| } | ||
|
|
||
| private static TemporalWorkerOptions PrepareOptions<TWorkflow>(TemporalWorkerOptions options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private static TemporalWorkerOptions PrepareOptions<TWorkflow>(TemporalWorkerOptions options) | |
| private static TemporalWorkerOptions PrepareWorkerOptions<TWorkflow>(TemporalWorkerOptions options) |
Pedantic, don't have to change
da013d2 to
bf2f78c
Compare
What was changed
Changed how resources are disposed inside
TemporalWorker. IfExecuteAsyncis called, then theDisposemethod does nothing and instead the resources are disposed at the end ofExecuteAsync. If it's not called, then resources are disposed withDisposecall as normal.Why?
Fixes crash that happens if
TemporalWorkeris disposed too early.Checklist
Closes [Bug] Segfault/crash occurs when using tasks without await #500
How was this tested:
Added testcase
WorkflowWorkerTests.ExecuteWorkflowAsync_PrematureDispose_WorkflowCompletes