CefSharp
CefSharp copied to clipboard
cefsharp
Fix non-thread-safety of TrySetResultAsync
The extension method TaskExtensions.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 PR adds a wrapper class around TaskCompletionSource (AsyncTaskCompletionSource), that takes care of calling TrySetResult in an async fashion while ensuring that only the first call is executed. I would really recommend using it instead of TaskCompletionSource each time TaskExtensions.TrySetResultAsync is 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).
:white_check_mark: Build CefSharp 65.0.0-CI2566 completed (commit https://github.com/cefsharp/CefSharp/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?
Well as you wish, that would be a good solution too.
I found that bug when using AsyncExtensions.SetCookieAsync(this ICookieManager cookieManager, string url, Cookie cookie); that would sometimes randomly return false, due to this bug (it uses TaskSetCookieCallback internally, which is one of the callback implementations modified by this PR). I did not try to trigger the bug for the other ones but in any case, the method TaskExtensions.TrySetResultAsync is flawed.
:white_check_mark: Build CefSharp 65.0.0-CI2567 completed (commit https://github.com/cefsharp/CefSharp/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 TaskExtensions.TrySetResultAsync is a bit dangerous: it does not behave like the .Net framework method TaskCompletionSource<>.TrySetResult it intends to replace (because of this bug), so it's misleading and error-prone (it's really easy to call it twice if you are not very careful, because that would not bother you for the origin method). As such, it should be removed/obsoleted as much as possible.
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.
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 https://github.com/cefsharp/CefSharp/commit/43c1ff3ed0fe418763f0f07c3124711f0657fa5e 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.