Skip to content
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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CefSharp.OffScreen/ChromiumWebBrowser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ public Task<Bitmap> ScreenshotAsync(bool ignoreExistingScreenshot = false, Popup
// Try our luck and see if there is already a screenshot, to save us creating a new thread for nothing.
var screenshot = ScreenshotOrNull(blend);

var completionSource = new TaskCompletionSource<Bitmap>();
var completionSource = new AsyncTaskCompletionSource<Bitmap>();

if (screenshot == null || ignoreExistingScreenshot)
{
Expand Down
25 changes: 14 additions & 11 deletions CefSharp.Test/Framework/AsyncExtensionFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,25 @@ public async Task DeleteCookiesReturnsTaskWhenReturnsFalse()
[Fact]
public async Task TaskDeleteCookiesCallbackOnComplete()
{
const int numberOfCookiesDeleted = 10;
for (var i = 0; i < 100; i++)
{
const int numberOfCookiesDeleted = 10;

var callback = new TaskDeleteCookiesCallback();
var callback = new TaskDeleteCookiesCallback();

//Execute OnComplete on seperate Thread as in practice will be called on the CEF IO Thread.
Task.Delay(100).ContinueWith(x =>
{
var c = (IDeleteCookiesCallback)callback;
//Execute OnComplete on seperate Thread as in practice will be called on the CEF IO Thread.
Task.Delay(100).ContinueWith(x =>
{
var c = (IDeleteCookiesCallback)callback;

c.OnComplete(numberOfCookiesDeleted);
c.Dispose();
}, TaskScheduler.Default);
c.OnComplete(numberOfCookiesDeleted);
c.Dispose();
}, TaskScheduler.Default);

var result = await callback.Task;
var result = await callback.Task;

Assert.Equal(numberOfCookiesDeleted, result);
Assert.Equal(numberOfCookiesDeleted, result);
}
}

[Fact]
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Callback/TaskCompletionCallback.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ namespace CefSharp
{
public class TaskCompletionCallback : ICompletionCallback
{
private readonly TaskCompletionSource<bool> taskCompletionSource;
private readonly AsyncTaskCompletionSource<bool> taskCompletionSource;

public TaskCompletionCallback()
{
taskCompletionSource = new TaskCompletionSource<bool>();
taskCompletionSource = new AsyncTaskCompletionSource<bool>();
}

void ICompletionCallback.OnComplete()
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Callback/TaskDeleteCookiesCallback.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ public class TaskDeleteCookiesCallback : IDeleteCookiesCallback
{
public const int InvalidNoOfCookiesDeleted = -1;

private readonly TaskCompletionSource<int> taskCompletionSource;
private readonly AsyncTaskCompletionSource<int> taskCompletionSource;
private volatile bool isDisposed;

public TaskDeleteCookiesCallback()
{
taskCompletionSource = new TaskCompletionSource<int>();
taskCompletionSource = new AsyncTaskCompletionSource<int>();
}

void IDeleteCookiesCallback.OnComplete(int numDeleted)
Expand Down
2 changes: 1 addition & 1 deletion CefSharp/Callback/TaskPrintToPdfCallback.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace CefSharp
{
public sealed class TaskPrintToPdfCallback : IPrintToPdfCallback
{
private readonly TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();
private readonly AsyncTaskCompletionSource<bool> taskCompletionSource = new AsyncTaskCompletionSource<bool>();

public Task<bool> Task
{
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Callback/TaskRegisterCdmCallback.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ namespace CefSharp
/// </summary>
public class TaskRegisterCdmCallback: IRegisterCdmCallback
{
private readonly TaskCompletionSource<CdmRegistration> taskCompletionSource;
private readonly AsyncTaskCompletionSource<CdmRegistration> taskCompletionSource;

public TaskRegisterCdmCallback()
{
taskCompletionSource = new TaskCompletionSource<CdmRegistration>();
taskCompletionSource = new AsyncTaskCompletionSource<CdmRegistration>();
}

void IRegisterCdmCallback.OnRegistrationComplete(CdmRegistration registration)
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Callback/TaskResolveCallback.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ namespace CefSharp
{
public class TaskResolveCallback : IResolveCallback
{
private readonly TaskCompletionSource<ResolveCallbackResult> taskCompletionSource;
private readonly AsyncTaskCompletionSource<ResolveCallbackResult> taskCompletionSource;

public TaskResolveCallback()
{
taskCompletionSource = new TaskCompletionSource<ResolveCallbackResult>();
taskCompletionSource = new AsyncTaskCompletionSource<ResolveCallbackResult>();
}

public void OnResolveCompleted(CefErrorCode result, IList<string> resolvedIpAddresses)
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Callback/TaskSetCookieCallback.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ namespace CefSharp
/// </summary>
public class TaskSetCookieCallback : ISetCookieCallback
{
private readonly TaskCompletionSource<bool> taskCompletionSource;
private readonly AsyncTaskCompletionSource<bool> taskCompletionSource;
private volatile bool isDisposed;

public TaskSetCookieCallback()
{
taskCompletionSource = new TaskCompletionSource<bool>();
taskCompletionSource = new AsyncTaskCompletionSource<bool>();
}

void ISetCookieCallback.OnComplete(bool success)
Expand Down
1 change: 1 addition & 0 deletions CefSharp/CefSharp.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
<ItemGroup>
<Compile Include="AsyncExtensions.cs" />
<Compile Include="BindingOptions.cs" />
<Compile Include="Internals\AsyncTaskCompletionSource.cs" />
<Compile Include="Callback\IAuthCallback.cs" />
<Compile Include="Callback\IBeforeDownloadCallback.cs" />
<Compile Include="Callback\ICallback.cs" />
Expand Down
43 changes: 43 additions & 0 deletions CefSharp/Internals/AsyncTaskCompletionSource.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// 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 asynchronous
/// fashion. This prevents the resulting Task continuations from being executed synchronously on the same
/// thread as the one completing the Task, which may be better when that thread is a CEF UI thread.
/// </summary>
/// <typeparam name="T"></typeparam>
public class AsyncTaskCompletionSource<T>
Copy link
Member

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.

Copy link
Author

@shkdee shkdee Apr 9, 2018

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.

Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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.

Copy link
Contributor

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 ?

Copy link
Member

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 ?

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

{
private readonly TaskCompletionSource<T> tcs = new TaskCompletionSource<T>();
private int resultSet = 0;

/// <summary>
/// Gets the <see cref="Task{TResult}"/> created by the wrapped <see cref="AsyncTaskCompletionSource{T}"/> instance.
/// </summary>
/// <seealso cref="TaskCompletionSource{TResult}.Task"/>
public Task<T> Task => tcs.Task;

/// <summary>
/// Sets the wrapped <see cref="TaskCompletionSource{T}"/> instance result in an asynchronous fashion.
/// This prevents the Task continuations from being executed synchronously on the same thread as the
/// one calling this method, which is required when this thread is a CEF UI thread.
/// </summary>
/// <param name="result">result</param>
public void TrySetResultAsync(T result)
{
if (Interlocked.Exchange(ref resultSet, 1) == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The 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);
}
}
}
}
8 changes: 7 additions & 1 deletion CefSharp/Internals/TaskExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,14 @@ public static TaskCompletionSource<TResult> WithTimeout<TResult>(this TaskComple
}

/// <summary>
/// <para>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// 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>
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Visitor/TaskCookieVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ namespace CefSharp
/// </summary>
public class TaskCookieVisitor : ICookieVisitor
{
private readonly TaskCompletionSource<List<Cookie>> taskCompletionSource;
private readonly AsyncTaskCompletionSource<List<Cookie>> taskCompletionSource;
private List<Cookie> list;

/// <summary>
/// Default constructor
/// </summary>
public TaskCookieVisitor()
{
taskCompletionSource = new TaskCompletionSource<List<Cookie>>();
taskCompletionSource = new AsyncTaskCompletionSource<List<Cookie>>();
list = new List<Cookie>();
}

Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Visitor/TaskNavigationEntryVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ namespace CefSharp
/// </summary>
public class TaskNavigationEntryVisitor : INavigationEntryVisitor
{
private TaskCompletionSource<List<NavigationEntry>> taskCompletionSource;
private AsyncTaskCompletionSource<List<NavigationEntry>> taskCompletionSource;
private List<NavigationEntry> list;

/// <summary>
/// Default constructor
/// </summary>
public TaskNavigationEntryVisitor()
{
taskCompletionSource = new TaskCompletionSource<List<NavigationEntry>>();
taskCompletionSource = new AsyncTaskCompletionSource<List<NavigationEntry>>();
list = new List<NavigationEntry>();
}

Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Visitor/TaskStringVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ namespace CefSharp
/// </summary>
public class TaskStringVisitor : IStringVisitor
{
private readonly TaskCompletionSource<string> taskCompletionSource;
private readonly AsyncTaskCompletionSource<string> taskCompletionSource;

/// <summary>
/// Default constructor
/// </summary>
public TaskStringVisitor()
{
taskCompletionSource = new TaskCompletionSource<string>();
taskCompletionSource = new AsyncTaskCompletionSource<string>();
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions CefSharp/Visitor/TaskWebPluginInfoVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ namespace CefSharp
{
public class TaskWebPluginInfoVisitor : IWebPluginInfoVisitor
{
private TaskCompletionSource<List<WebPluginInfo>> taskCompletionSource;
private AsyncTaskCompletionSource<List<WebPluginInfo>> taskCompletionSource;
private List<WebPluginInfo> list;

public TaskWebPluginInfoVisitor()
{
taskCompletionSource = new TaskCompletionSource<List<WebPluginInfo>>();
taskCompletionSource = new AsyncTaskCompletionSource<List<WebPluginInfo>>();
list = new List<WebPluginInfo>();
}

Expand Down