semantic-kernel icon indicating copy to clipboard operation
semantic-kernel copied to clipboard

Published 20 hours ago • verified publisher icon microsoft

Adding ConfigureAwait(false) to SDK projects

Open shawncal opened this issue 2 years ago • 3 comments

Motivation and Context

It's recommended that .ConfigureAwait(false) be added to all awaits in SDK-style projects, to prevent deadlocks in calling apps.

Description

  • This change re-enables the .editorconfig rules that check for, and enforce, a ConfigureAwait(true or false), in projects under the dotnet/ folder.
  • Sample apps (under samples/) are not affected
  • Unit test projects under dotnet/ are excluded via <NoWarn> tags in the csproj

Contribution Checklist

  • [x] The code builds clean without any errors or warnings
  • [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
  • [x] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
  • [x] All unit tests pass, and I have added new tests where possible
  • [x] I didn't break anyone :smile:

shawncal avatar Apr 20 '23 02:04 shawncal

One important question to answer here...

Developers can supply delegates that will end up being invoked by the library. Is there any expectation that those delegates will be invoked back on the original SynchronizationContext that created them? eg Might someone reasonably expect that they can supply a delegate that directly updates controls on their UI?

stephentoub avatar Apr 21 '23 11:04 stephentoub

LGTM

💡You could use Fody.ConfigureAwait to make that implicit, since it should be the default for new code anyway.

MovGP0 avatar Apr 21 '23 13:04 MovGP0

Developers can supply delegates that will end up being invoked by the library.

Yes that is an issue here. With the ConfigureAwait(false) the code will potentially return in a different thread, such that the delegate will also be called from a different thread; and the awaited value might also happen on a different thread. Client libraries, which are not written in a thread-save manner, will likely be effected by this change.

My personal opinion is that this would be an issue with the client that should be fixed; rather than an issue that should be blamed on the library.

MovGP0 avatar Apr 21 '23 13:04 MovGP0