-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix non-thread-safety of TrySetResultAsync #2349
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// Copyright © 2010-2017 The CefSharp Authors. All rights reserved. | ||
// | ||
// Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. | ||
|
||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace CefSharp | ||
{ | ||
/// <summary> | ||
/// Wraps <see cref="TaskCompletionSource{T}"/> by providing a way to set its result in an async fashion. | ||
/// This prevents the Task Continuation being executed sync on the same thread, which may be required when | ||
/// continuations must not run on CEF UI threads. | ||
/// </summary> | ||
/// <typeparam name="T"></typeparam> | ||
public class AsyncTaskCompletionSource<T> | ||
{ | ||
private readonly TaskCompletionSource<T> tcs = new TaskCompletionSource<T>(); | ||
private int resultSet = 0; | ||
|
||
/// <summary> | ||
/// Gets the <see cref="Task{TResult}"/> created by this <see cref="AsyncTaskCompletionSource{T}"/> | ||
/// </summary> | ||
/// <seealso cref="TaskCompletionSource{TResult}.Task"/> | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By "a relevant resource", you mean regarding executing code on CEF UI threads ? So something like the general usage guide of the wiki ? |
||
/// sync on the same thread. This is required otherwise contintinuations will happen on CEF UI threads. | ||
/// </summary> | ||
/// <param name="result">result</param> | ||
public void TrySetResultAsync(T result) | ||
{ | ||
if (Interlocked.Exchange(ref resultSet, 1) == 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code consistently, braces required even for single line if block. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
System.Threading.Tasks.Task.Factory.StartNew(() => tcs.TrySetResult(result), | ||
CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,8 +89,14 @@ public static TaskCompletionSource<TResult> WithTimeout<TResult>(this TaskComple | |
} | ||
|
||
/// <summary> | ||
/// <para> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we go with your proposal, can this method be deleted? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it could, it's just still used in a few places in the examples. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll let you do that then. |
||
/// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed sync on the same thread | ||
/// This is required otherwise contintinuations will happen on CEF UI threads | ||
/// This is required otherwise contintinuations will happen on CEF UI threads. | ||
/// </para> | ||
/// <para> | ||
/// USAGE WARNING: if this method is called twice on the same task completion source, the result it not deterministic. | ||
/// Use it with care, or use <see cref="AsyncTaskCompletionSource{T}"/>. | ||
/// </para> | ||
/// </summary> | ||
/// <typeparam name="TResult">Generic param</typeparam> | ||
/// <param name="taskCompletionSource">tcs</param> | ||
|
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.
This can probably be made internal.
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.
It's also used in the project
CefSharp.OffScreen
(once), so it cannot actually.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok no worries, will have to remain public.
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.
guys why dont u use the default TaskCompletionSource and create it with TaskCreationOptions.RunContinuationsAsync ?
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.
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