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

[System]: Use new WebCompletionSource instead of TaskCompletionSource. #7293

Merged
merged 2 commits into from
Feb 28, 2018

Conversation

baulig
Copy link
Contributor

@baulig baulig commented Feb 26, 2018

This new helper class uses TaskCompletionSource<> internally, but instead
of using TrySetException(), it sets a result containing an ExceptionDispatchInfo.

The problem with using TrySetException() is that it wraps the exception
object in an AggregateException, that would be unobserved if the caller
throws the original exception.

#7194

@baulig
Copy link
Contributor Author

baulig commented Feb 26, 2018

@monojenkins build pkg

@baulig
Copy link
Contributor Author

baulig commented Feb 26, 2018

@monojenkins backport 2018-02

@baulig
Copy link
Contributor Author

baulig commented Feb 26, 2018

@monojenkins build pkg

…rce<>`.

This new helper class uses `TaskCompletionSource<>` internally, but instead
of using TrySetException(), it sets a result containing an `ExceptionDispatchInfo`.

The problem with using TrySetException() is that it wraps the exception
object in an `AggregateException`, that would be unobserved if the caller
throws the original exception.
@baulig baulig changed the title [System]: Use new WebCompletionSource instead of TaskCompletionSource<>. [System]: Use new WebCompletionSource instead of TaskCompletionSource. Feb 26, 2018
@baulig
Copy link
Contributor Author

baulig commented Feb 26, 2018

@monojenkins build pkg

@baulig
Copy link
Contributor Author

baulig commented Feb 26, 2018

@monojenkins backport 2018-02

completion = new TaskCompletionSource<Result> ();
}

public bool SetCompleted ()
Copy link
Member

Choose a reason for hiding this comment

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

Should be called TrySetCompleted

return completion.TrySetResult (new Result (1, null));
}

public bool SetCanceled ()
Copy link
Member

Choose a reason for hiding this comment

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

Should be called TrySetCanceled


public bool IsCompleted => completion.Task.IsCompleted;

public void Throw ()
Copy link
Member

Choose a reason for hiding this comment

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

This is actually ThrowOnError, right?


class Result
{
public int State {
Copy link
Member

Choose a reason for hiding this comment

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

Use enum instead of int

@marek-safar marek-safar merged commit 8facc4a into mono:master Feb 28, 2018
@marek-safar
Copy link
Member

@monojenkins backport 2018-02

akoeplinger pushed a commit that referenced this pull request Feb 28, 2018
@baulig baulig deleted the work-unobserved-exceptions branch March 16, 2018 19:51
UnityAlex pushed a commit to Unity-Technologies/mono that referenced this pull request Aug 29, 2019
…rce`. (mono#7293)

This new helper class uses `TaskCompletionSource<>` internally, but instead
of using TrySetException(), it sets a result containing an `ExceptionDispatchInfo`.

The problem with using TrySetException() is that it wraps the exception
object in an `AggregateException`, that would be unobserved if the caller
throws the original exception.
UnityAlex pushed a commit to Unity-Technologies/mono that referenced this pull request Sep 3, 2019
…rce`. (mono#7293)

This new helper class uses `TaskCompletionSource<>` internally, but instead
of using TrySetException(), it sets a result containing an `ExceptionDispatchInfo`.

The problem with using TrySetException() is that it wraps the exception
object in an `AggregateException`, that would be unobserved if the caller
throws the original exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants