Fix non-thread-safety of TrySetResultAsync #2349
Conversation
The extension method TrySetResultAsync has non-deterministic results when called twice on the same instance, even on the same thread: it creates one task for each call, which may not run in their creation order. This commit adds a wrapper around TaskCompletionSource, that ensures that only the first call to TrySetResultAsync is really executed. It also updates the test case that could be violated with the previous behavior (but that was hard to trigger with only one test).
fix code style
|
✅ Build CefSharp 65.0.0-CI2566 completed (commit d025d3e68e by @shkdee) |
|
Ideally we'd update to .Net 4.6 and remove TrySetResultAsync. Other than the single test case what real world use case are you experiencing an issue with? Which CEF callback is calling TrySetResultAsync twice? |
| @@ -89,8 +89,14 @@ public static TaskCompletionSource<TResult> WithTimeout<TResult>(this TaskComple | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// <para> | |||
There was a problem hiding this comment.
If we go with your proposal, can this method be deleted?
There was a problem hiding this comment.
I guess it could, it's just still used in a few places in the examples.
There was a problem hiding this comment.
We can look at upgrading the example projects to .Net 4.6 if required.
Only the core projects need to remain as .Net 4.5.2
There was a problem hiding this comment.
I'll let you do that then.
I also just noticed it was used in two places in the CefSharp.Core project, and I'm much less familiar with C++/CLI to handle that case.
| /// <param name="result">result</param> | ||
| public void TrySetResultAsync(T result) | ||
| { | ||
| if (Interlocked.Exchange(ref resultSet, 1) == 0) |
There was a problem hiding this comment.
Code consistently, braces required even for single line if block.
| /// continuations must not run on CEF UI threads. | ||
| /// </summary> | ||
| /// <typeparam name="T"></typeparam> | ||
| public class AsyncTaskCompletionSource<T> |
There was a problem hiding this comment.
This can probably be made internal.
There was a problem hiding this comment.
It's also used in the project CefSharp.OffScreen (once), so it cannot actually.
There was a problem hiding this comment.
Last I checked IntetnalsVisibleTo was working for all the c# projects, just doesn't work with the VC++ ones, so we do have to leave it public if it's going to be used in C++.
https://github.com/cefsharp/CefSharp/blob/master/CefSharp/Properties/AssemblyInfo.cs#L26
There was a problem hiding this comment.
Oh sorry, I missed the InternalsVisibleTo. Well then it's ok to make it internal except if we also want to replace the two usages of TaskExtensions::TrySetResultAsync which are in the C++ project.
There was a problem hiding this comment.
Ok no worries, will have to remain public.
There was a problem hiding this comment.
guys why dont u use the default TaskCompletionSource and create it with TaskCreationOptions.RunContinuationsAsync ?
There was a problem hiding this comment.
guys why dont u use the default TaskCompletionSource and create it with TaskCreationOptions.RunContinuationsAsync ?
TaskCreationOptions.RunContinuationsAsync was added in .Net 4.6
We are still supporting .Net 4.5.2 (no plans to change this)
One day I'll get around to updating the .Net Core builds to use RunContinuationsAsync
| public Task<T> Task => tcs.Task; | ||
|
|
||
| /// <summary> | ||
| /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed |
There was a problem hiding this comment.
This needs rewording. I had to read it three times, your new sentence about sync on same thread could be clearer. Should probably also link to a relevant resource, I should have done that previously.
There was a problem hiding this comment.
By "a relevant resource", you mean regarding executing code on CEF UI threads ? So something like the general usage guide of the wiki ?
|
Well as you wish, that would be a good solution too. I found that bug when using |
|
✅ Build CefSharp 65.0.0-CI2567 completed (commit 903e8ef976 by @shkdee) |
|
Upgrading to .Net 4.6 currently isn't an option for the core projects. It looks like there are only a couple of classes that have the potential for this bug. Looks like only the callbacks have the potential to make multiple calls. I guess the question is should we limit the scope of this PR to only the relevant classes? Will have a think about it, comments welcome. |
|
For that PR only, it would be easier and much less breaking to limit it to the relevant classes. But in the long term, for the CefSharp project, my opinion on the matter is that the current method |
|
TrySetResultAsync is only intended to be used within the context of CefSharp it's self, so the scope for error in usage is very small. I think upgrading the example projects to .Net 4.6 and removing is usage from those examples makes a lot of sense, we shouldn't be encouraging others to use the same method. If TrySetResultAsync does remain, then it should be renamed for clarity, it's sole purpose is to ensure the continuation happens on the thread pool and not on the CEF UI thread. |
…r called Add IsDisposed property to ICompletionCallback, IPrintToPdfCallback, IRegisterCdmCallback, IResolveCallback Should resolve #2349 until the code review can be finished
|
I feel this needs more work that I have time for just at the moment, to resolve the issue with the callbacks I've applied a very simple bool check, see commit 43c1ff3 for details. The complete and dispose methods should always be called on the same thread (dispose is slightly different to the standard .net dispose in this context as it's being called from unmanged code when the parent ref pointer goes out of scope). This should resolve the short term issue in the next release and I'll come back to this when I have some time. |
|
I'll resolve the merge conflicts when it comes time to merge this. |
… a long term solution for #2349 is devised - Add OnComplete check to all the TaskCompletionSource based callbacks that could potentially have executed TrySetResultAsync twice, which could on rare occasions execute in a non deterministic fashion (The Disposed execution of TrySetResultAsync called before the OnComplete one). See #2349 for a more detailed proposal and one potential long term solution (upgrading to .Net 4.6 is the best option, though not practical now, will update the example projects shortly). - Update callbacks for consistency, all now implement IDisposable and have an IsDisposed
|
#4482 updates to |
The extension method
TaskExtensions.TrySetResultAsynchas non-deterministic results when called twice on the same instance, even on the same thread: it creates one task for each call, which may not run in their creation order.This PR adds a wrapper class around
TaskCompletionSource(AsyncTaskCompletionSource), that takes care of callingTrySetResultin an async fashion while ensuring that only the first call is executed. I would really recommend using it instead ofTaskCompletionSourceeach timeTaskExtensions.TrySetResultAsyncis used (even if it is called only once in any logical workflow), because this non-determinism can cause very subtle and hard-to-track bugs.This PR also updates the test case that could be violated with the previous behavior (but that was hard to trigger with only one test).