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

Deadlocks and CancellationToken #44

Closed
wants to merge 2 commits into from

Conversation

sergeylopatyuk
Copy link

Added ConfigureAwait(false) to prevent deadlocks.
Added CancelationToken to methods.
Please accept pull request.

@wo80
Copy link
Collaborator

wo80 commented Jul 16, 2019

Thanks.

Could you explain the use case for CancellationToken? Usually, you'd use it to cancel a long running task. Not sure if a single request to the MusicBrainz API would be considered long running.

@sergeylopatyuk
Copy link
Author

Nevertheless it is good to have this opportunity, turning to the 3rd party can have unpredictable consequences. Also in my case, I need it. Who needs will can use, who does not - can ignore.
Sorry for my English...

@wo80
Copy link
Collaborator

wo80 commented Jul 16, 2019

Well, to extend an API just because it does no harm isn't a good argument.

Also in my case, I need it.

Could you then please explain your use case?

Using ConfigureAwait(false) is definitly a good addition and I will merge the change. But I'm not so sure about the usage of CancellationToken.

@sergeylopatyuk
Copy link
Author

It is accepted that if an asynchronous operation can receive a token, it should be possible to transfer it, we cannot know how the library will be used, with what parameters and with what quality of the communication channel. Also, since this is a third party, we cannot know what problems are possible, anything happens. By default, the HttpClient timeout is 100 seconds and it is not subject to any significant problems, but the ability to cancel the operation or specify a different timeout, in my opinion, should be. I have no other weighty arguments.

@wo80
Copy link
Collaborator

wo80 commented Jul 17, 2019

Fair enough! I'll leave this pending for a couple of days to see if there are any concerns.

/// </summary>
/// <param name="s">Source line.</param>
/// <returns></returns>
public static string Quote(this string s)
Copy link
Collaborator

@wo80 wo80 Sep 6, 2019

Choose a reason for hiding this comment

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

Since the extension is not connected to the rest of the library, I would prefer this to be moved back to the example project.

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