Fix non-thread-safety of TrySetResultAsync by shkdee · Pull Request #2349 · cefsharp/CefSharp

shkdee · 2018-04-09T11:17:32Z

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).

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

AppVeyorBot · 2018-04-09T11:21:11Z

✅ Build CefSharp 65.0.0-CI2566 completed (commit d025d3e68e by @shkdee)

amaitland · 2018-04-09T11:55:32Z

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?

amaitland · 2018-04-09T12:03:19Z

        }

        /// <summary>
+        /// <para>


If we go with your proposal, can this method be deleted?

I guess it could, it's just still used in a few places in the examples.

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

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.

amaitland · 2018-04-09T12:04:26Z

+        /// <param name="result">result</param>
+        public void TrySetResultAsync(T result)
+        {
+            if (Interlocked.Exchange(ref resultSet, 1) == 0)


Code consistently, braces required even for single line if block.

amaitland · 2018-04-09T12:11:48Z

+    /// continuations must not run on CEF UI threads.
+    /// </summary>
+    /// <typeparam name="T"></typeparam>
+    public class AsyncTaskCompletionSource<T>


This can probably be made internal.

It's also used in the project CefSharp.OffScreen (once), so it cannot actually.

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

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.

Ok no worries, will have to remain public.

guys why dont u use the default TaskCompletionSource and create it with TaskCreationOptions.RunContinuationsAsync ?

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

amaitland · 2018-04-09T12:18:28Z

+        public Task<T> Task => tcs.Task;
+
+        /// <summary>
+        /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed


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.

By "a relevant resource", you mean regarding executing code on CEF UI threads ? So something like the general usage guide of the wiki ?

shkdee · 2018-04-09T12:30:40Z

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.

AppVeyorBot · 2018-04-09T13:00:19Z

✅ Build CefSharp 65.0.0-CI2567 completed (commit 903e8ef976 by @shkdee)

amaitland · 2018-04-09T13:07:51Z

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.

shkdee · 2018-04-09T13:27:52Z

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.

amaitland · 2018-04-09T14:31:40Z

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

amaitland · 2018-04-30T04:30:12Z

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.

amaitland · 2018-04-30T04:30:30Z

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

amaitland · 2023-04-21T09:12:34Z

#4482 updates to .Net 4.6.2 and removes TrySetResultAsync.

shkdee added 2 commits April 9, 2018 13:02

code style

00c27e5

fix code style

amaitland reviewed Apr 9, 2018

View reviewed changes

comments and code style

ec98f0d

amaitland closed this in 43c1ff3 Apr 30, 2018

amaitland reopened this Apr 30, 2018

amaitland added a commit that referenced this pull request May 9, 2018

AsyncExtensionFacts - Add test case for #2349

dd42fdb

amaitland closed this Apr 21, 2023

Uh oh!

Conversation

shkdee commented Apr 9, 2018

Uh oh!

AppVeyorBot commented Apr 9, 2018

Uh oh!

amaitland commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shkdee Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shkdee commented Apr 9, 2018

Uh oh!

AppVeyorBot commented Apr 9, 2018

Uh oh!

amaitland commented Apr 9, 2018

Uh oh!

shkdee commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaitland commented Apr 9, 2018

Uh oh!

amaitland commented Apr 30, 2018

Uh oh!

amaitland commented Apr 30, 2018

Uh oh!

amaitland commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shkdee Apr 9, 2018 •

edited

Loading

shkdee commented Apr 9, 2018 •

edited

Loading