Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Jan 24, 2022

As described in #12637 (comment), part of #11142 created a problematic change in behaviour about when Async.RunSynchronously uses the current thread. Using the current thread is godo for performance and stacks but can cause deadlock if the current thread is relied on to process part of the async computation.

  1. Prior to Await completed Tasks on the same thread. #11142 the current thread was used if SynchronizationContext is null (and no timeout specified)

  2. After Await completed Tasks on the same thread. #11142 the current thread is used if Thread.CurrentThread.IsThreadPoolThread is true (and no timeout specified).

This change was not conservative - we now start on the current thread more often than we did before, specifically when we have non-null SynchronizationContext, yet CurrentThread.IsThreadPoolThread is true.

The idea of #11142 was not bad - it tries to make sure the computation always runs in the thread pool. However sometimes thread-pool threads have non-null SynchronizationContext.Current, and this is an indication that starting the async computation on the current thread is not acceptable.

As a conservative fix, we clarify the spec of RunSynchronously as follows:

The computation is started on the current thread if and only if all of the following hold: SynchronizationContext.Current is null, Thread.CurrentThread.IsThreadPoolThread is true and no timeout is specified. If any of these conditions are false it is started by queueing a new work item in the thread pool. This means that the computation will always be started on a thread-pool thread where the SynchronizationContext.Current is null.

This PR adjusts the code to match that.

Testing

Running without first adjusting tests to see if our existing tests clarify this.

@dsyme dsyme changed the title [WIPRevert part of 11142 [WIP] Change part of 11142 to be more conservative Jan 24, 2022
@dsyme dsyme changed the title [WIP] Change part of 11142 to be more conservative [WIP] Change Async.RunSynchronously back to be more conservative about using current thread (reverting part of 11142) Jan 24, 2022
@dsyme dsyme changed the title [WIP] Change Async.RunSynchronously back to be more conservative about using current thread (reverting part of 11142) [WIP] Make Async.RunSynchronously more conservative about using current thread (reverting part of 11142) Jan 24, 2022
@KevinRansom
Copy link
Contributor

KevinRansom commented Jan 24, 2022

Release Notes:
Package: FSharp.Core
Description: Relax RunSynchronously reliance on current thread, eliminates a source of deadlocks
Issue: #2577
Executing a list of asynchronous computations hangs after updating to FSharp.Core 5.0.2

@dsyme
Copy link
Contributor Author

dsyme commented Jan 24, 2022

I'll add a test for this

@dsyme dsyme changed the title [WIP] Make Async.RunSynchronously more conservative about using current thread (reverting part of 11142) Make Async.RunSynchronously more conservative about using current thread (reverting part of 11142) Jan 26, 2022
@dsyme dsyme merged commit cfc9d3b into dotnet:main Jan 26, 2022
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