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

Avoid using async functions from synchronous code #302

Open
wants to merge 1 commit into
base: dev1.x
Choose a base branch
from

Conversation

pergardebrink
Copy link

@pergardebrink pergardebrink commented Feb 17, 2020

This changes the retrytimeout to use Thread.Sleep as we cannot use Task.Delay in this context due to risk of deadlocks.

@pergardebrink pergardebrink marked this pull request as ready for review February 17, 2020 13:39
@MichaCo
Copy link
Owner

MichaCo commented Feb 17, 2020

I don't think it is a good idea to use Thread.Sleep. That would suspend the thread which might run many tasks in the host application and could cause even more harm.

I think I'd either use Task.Delay(x).GetAwaiter().GetResult() or remove the delay between retries all together because of possibly too many side effects.

@pergardebrink
Copy link
Author

I think I'd either use Task.Delay(x).GetAwaiter().GetResult() or remove the delay between retries all together because of possibly too many side effects.

Wouldn't that also block the current thread in the same way as a Thread.Sleep would do, as the GetResult() call is blocking the current thread until the Task ends?

@pergardebrink
Copy link
Author

I think I'd either use Task.Delay(x).GetAwaiter().GetResult() or remove the delay between retries all together because of possibly too many side effects.

Wouldn't that also block the current thread in the same way as a Thread.Sleep would do, as the GetResult() call is blocking the current thread until the Task ends?

Looked a bit more into this for a reference for my reasoning here. David Fowler at Microsoft has a great collection on guidance on async programming. He has a section on "sync over async" about that invoking an async method (for example Task.Delay) and then blocking the thread while waiting (GetAwaiter().GetResult()) for it to complete could be worse than just calling the synchronous version (Thread.Sleep):
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#warning-sync-over-async

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.

None yet

2 participants