Fix 581 - Async.RunSynchronously tries to reuse current thread

[radekm]

Fixes #581 - Async.RunSynchronously tries to reuse the current ThreadPool thread for running the computation (originally the computation was always performed in another ThreadPool thread).

Currently there are two problems:

1. The code doesn't use Thread.IsThreadPoolThread since it isn't available in all profiles (eg. in profile 47). Instead value of Thread.IsThreadPoolThread is approximated by SynchronizationContext.Current = null.

2. Current ThreadPool thread is reused only if no timeout was given to Async.RunSynchronously. The reason is that some thread is needed to cancel the computation when the time runs out and the current thread seems as a good candidate for this task. In some profiles I could use System.Threading.Timer or CancellationTokenSource.CancelAfter (internally uses Timer) but they don't ensure that cancellation is performed when ThreadPool is busy. For example following code

open System
open System.Threading

let startWork (token : CancellationToken) (ev : CountdownEvent) n =
    let work () =
        Console.Write("{0}|", Thread.CurrentThread.ManagedThreadId)
        while not token.IsCancellationRequested do
            Thread.Sleep(10)
        Console.Write(".")
        ev.Signal() |> ignore
    for _ in 1..n do
        if ThreadPool.QueueUserWorkItem(fun _ -> work ()) then
            ev.AddCount()

[<EntryPoint>]
let main argv = 
    use cts = new CancellationTokenSource(2000)
    // `timer` ensures that action from `Timer` from `cts` is placed at the end of `ThreadPool` queue.
    // Note that `timer` declaration must follow `cts` declaration and timeouts must be same.
    use timer = new Timer((fun _ -> Console.Write("timer {0}|", Thread.CurrentThread.ManagedThreadId)), null, 2000, -1)
    use finished = new CountdownEvent(1)

    let sw = System.Diagnostics.Stopwatch()
    sw.Start()
    startWork cts.Token finished 200
    finished.Signal() |> ignore
    Console.Write("# tasks {0}|", finished.CurrentCount)
    finished.Wait()
    sw.Stop()

    printfn "Elapsed: %A" sw.Elapsed.TotalSeconds
    0

requests cancellation after 2 seconds but cancellation actually happens after 193 seconds on my machine and if the ThreadPool reaches its maximal capacity (eg. by ThreadPool.SetMaxThreads(4, 4)) then the cancellation never happens.

The third option is to create a single special thread which will be dedicated for cancellation (ie. for calling CancellationTokenSource.Cancel). This special thread could be used by all calls to Async.RunSynchronously.

[msftclas]

Hi @radekm, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[vasily-kirichenko]

Why not match timeout as well?

[radekm]

Good point, I'll fix that.

[dsyme]

This enhancement looks correct to me. Would welcome additional reviewers.

[forki]

this test doesn't assert anything, right?

[radekm]

Yes, there is no assert. The purpose of this test is to ensure that the computation completes quickly (originally it took more than 1 minute with 100 actions - now there are 1000 actions and it takes less than 1 second) - otherwise TimeoutException would be raised.

[forki]

Ok so the assertion is "no timeout" that's pretty nice, but maybe a small
comment for clarification would help.
On Dec 2, 2015 22:13, "radekm" notifications@github.com wrote:

In
src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs
#678 (comment):

@@ -80,6 +80,15 @@ type AsyncType() =
()

 [<Test>]

• member this.AsyncRunSynchronouslyReusesThreadPoolThread() =

•    let action = async { async { () } |> Async.RunSynchronously }
  

•    let computation =
  

•        [| for i in 1 .. 1000 -> action |]
  

•        |> Async.Parallel
  

•    Async.RunSynchronously(computation, timeout = 1000)
  

•    |> ignore
  

Yes, there is no assert. The purpose of this test is to ensure that the
computation completes quickly (originally it took more than 1 minute with
100 actions - now there are 1000 actions and it takes less than 1 second) -
otherwise TimeoutException would be raised.

—
Reply to this email directly or view it on GitHub
https://github.com/Microsoft/visualfsharp/pull/678/files#r46476056.

[radekm]

Ok, I'll add it.

[radekm]

I have improved the test AsyncRunSynchronouslyReusesThreadPoolThread by adding a comment and enclosing the call which may raise TimeoutException in Assert.DoesNotThrow

[jaredpar]

In console applications SynchronizationContext.Current will be null for the main application thread. How will that affect this change?

[dsyme]

Those applications will now run the computation on the main thread (instead of blocking that thread and running the application on a background thread). That is, I believe, intended.

[KevinRansom]

This looks great.