Skip to content

Remove use of ConfigureAwait from Microsoft.Extensions.AI.dll for AIFunction invocations #6250

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

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Apr 4, 2025

We try to use ConfigureAwait(false) throughout our libraries. However, we exempt ourselves from that in cases where user code is expected to be called back from within the async code, and there's a reasonable presumption that such code might care about the synchronization context. AIFunction fits that bill. And FunctionInvokingChatClient needs to invoke such functions, which means that we need to be able to successfully flow the context from where user code calls Get{Streaming}ResponseAsync through into wherever a FunctionInvokingChatClient is in the middleware pipeline. We could try to selectively avoid ConfigureAwait(false) on the path through middleware that could result in calls to FICC.Get{Streaming}ResponseAsync, but that's fairly brittle and hard to maintain. Instead, this PR just removes ConfigureAwait use from the M.E.AI library. It also fixes a few places where tasks were explicitly being created and queued to the thread pool.

Microsoft Reviewers: Open in CodeFlow

…unction invocations

We try to use ConfigureAwait(false) throughout our libraries. However, we exempt ourselves from that in cases where user code is expected to be called back from within the async code, and there's a reasonable presumption that such code might care about the synchronization context. AIFunction fits that bill. And FunctionInvokingChatClient needs to invoke such functions, which means that we need to be able to successfully flow the context from where user code calls Get{Streaming}ResponseAsync through into wherever a FunctionInvokingChatClient is in the middleware pipeline. We could try to selectively avoid ConfigureAwait(false) on the path through middleware that could result in calls to FICC.Get{Streaming}ResponseAsync, but that's fairly brittle and hard to maintain. Instead, this PR just removes ConfigureAwait use from the M.E.AI library. It also fixes a few places where tasks were explicitly being created and queued to the thread pool.
@stephentoub stephentoub requested a review from a team as a code owner April 4, 2025 21:39
@github-actions github-actions bot added the area-ai Microsoft.Extensions.AI libraries label Apr 4, 2025
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Apr 7, 2025

However, we exempt ourselves from that in cases where user code is expected to be called back from within the async code, and there's a reasonable presumption that such code might care about the synchronization context.

I think I need to better understand this exemption and its implications. For example, one could argue that JsonSerializer.SerializeAsync does call back to user code by virtue of it accepting registrations of custom converters. That logic could be extended to pretty much any async component accepting abstractions.

@stephentoub
Copy link
Member Author

It's the "and there's a reasonable presumption that such code might care about the synchronization context" part.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Apr 7, 2025

And what decides that presumption I guess is the fact that both here and in S.L.AsyncEnumerable we're unambiguously passing a single delegate for execution?

@stephentoub
Copy link
Member Author

Yes. It's subjective, how likely is it that it'll really be an issue, that someone has a reasonable assumption their code will be invoked in a particular context.

@stephentoub stephentoub merged commit 84d09b7 into dotnet:main Apr 7, 2025
7 checks passed
@stephentoub stephentoub deleted the fixfunctioninvocation branch April 7, 2025 11:11
joperezr pushed a commit to joperezr/extensions that referenced this pull request Apr 8, 2025
…unction invocations (dotnet#6250)

We try to use ConfigureAwait(false) throughout our libraries. However, we exempt ourselves from that in cases where user code is expected to be called back from within the async code, and there's a reasonable presumption that such code might care about the synchronization context. AIFunction fits that bill. And FunctionInvokingChatClient needs to invoke such functions, which means that we need to be able to successfully flow the context from where user code calls Get{Streaming}ResponseAsync through into wherever a FunctionInvokingChatClient is in the middleware pipeline. We could try to selectively avoid ConfigureAwait(false) on the path through middleware that could result in calls to FICC.Get{Streaming}ResponseAsync, but that's fairly brittle and hard to maintain. Instead, this PR just removes ConfigureAwait use from the M.E.AI library. It also fixes a few places where tasks were explicitly being created and queued to the thread pool.
joperezr pushed a commit to joperezr/extensions that referenced this pull request Apr 8, 2025
…ns.AI.dll for AIFunction...

Remove use of ConfigureAwait from Microsoft.Extensions.AI.dll for AIFunction invocations (dotnet#6250)

We try to use ConfigureAwait(false) throughout our libraries. However, we exempt ourselves from that in cases where user code is expected to be called back from within the async code, and there's a reasonable presumption that such code might care about the synchronization context. AIFunction fits that bill. And FunctionInvokingChatClient needs to invoke such functions, which means that we need to be able to successfully flow the context from where user code calls Get{Streaming}ResponseAsync through into wherever a FunctionInvokingChatClient is in the middleware pipeline. We could try to selectively avoid ConfigureAwait(false) on the path through middleware that could result in calls to FICC.Get{Streaming}ResponseAsync, but that's fairly brittle and hard to maintain. Instead, this PR just removes ConfigureAwait use from the M.E.AI library. It also fixes a few places where tasks were explicitly being created and queued to the thread pool.
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ai Microsoft.Extensions.AI libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants