Skip to content

Conversation

@tebeco
Copy link
Contributor

@tebeco tebeco commented Nov 3, 2017

Should never use async void

I tried other scenario but that seems to endup in deadlock or the WebHostBuilder is not happy
I ended up avoiding

Not working so far:

public async Task Configure(IApplicationBuilder app, IHostingEnvironment env)
{
  await Electron.WindowManager.CreateWindowAsync()
}
public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
  Electron.WindowManager.CreateWindowAsync().Start()
}
public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
  Electron.WindowManager.CreateWindowAsync().RunSynchronously()
}

@yeskunall
Copy link

I'm just going to place this quote from this article here in case anyone is wondering why this is useful:

Async void methods have different error-handling semantics. When an exception is thrown out of an async Task or async Task method, that exception is captured and placed on the Task object. With async void methods, there is no Task object, so any exceptions thrown out of an async void method will be raised directly on the SynchronizationContext that was active when the async void method started.

... also because Electron.WindowManager.CreateWindowAsync seems to be a CPU-bound task, spawning the work off on another thread with Task.Run will ensure responsiveness.

@tebeco
Copy link
Contributor Author

tebeco commented Nov 3, 2017

Well the question is
should/does AspnetCore support Async scenario while Configure()

@robertmuehsig robertmuehsig requested review from GregorBiswanger and robertmuehsig and removed request for GregorBiswanger November 3, 2017 22:30
@robertmuehsig robertmuehsig merged commit 9ed5738 into ElectronNET:master Nov 4, 2017
@robertmuehsig
Copy link
Collaborator

The normal Configure() is not async as well, so it seems your suggestion is "better" with the Task.Run...

@tebeco tebeco deleted the removing_async_void branch November 6, 2017 20:08
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.

3 participants