-
Notifications
You must be signed in to change notification settings - Fork 781
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
[FS-1028] - Implement Async.StartImmediateAsTask #2534
Conversation
The code looks ok though the tests indicate some failures? |
Yes kinda weird. The test failing is |
@OnurGumus I'll close and reopen to re-run tests. But I think more tests must be failing - usually each of part1, part2 and part3 indicate different failures For example I expect you will need to update the API surface area tests? |
I will check it again, but that's gonna be on the weekend again:) |
@dotnet-bot test this please |
Sorry for not dealing with this yet. I forgot to update surface area tests. I will check it asap. |
ping |
I just want to highlight a potential problem that also exists for Async.StartImmediate. For example:
If we use async everywhere then everything works perfectly. That is no thread switch occurs. However if start to use System.Threading.Tasks.Task in between as in above example, then a thread switch occurs. The async module is a little bit complicated for me with a lot of "trambolines" involved. |
@OnurGumus I think it's just luck or which thread pool thread gets chosen. When i tried it I got
|
@dsyme Which example have you tried? If you have used above example of course a thread switch occurs because tasks are involved. But the key is if you try below:
Then no thread switch occurs as there are no tasks involved. Hence we achieve our goal. |
Thread switches occur in your sample - the Async.Sleep calls are ultimately implemented via by a timer callback which may get scheduled on a different thread (unless the originating code was a thread with a non-null SynchronizationContext, e.g. a UI thread). When I compile this code
I get
|
@dsyme : Are you sure? Because that's weird. I am getting
repeatedly every time. As a proof see below gif file: I am using .NET 4.7 with F# 4.1 Release build 64 bit (though other configurations also yield the same result). |
@OnurGumus That's just luck with regard to the thread pool threads (e.g. thread 4) used for the timer callbacks. |
@dsyme , I see. Then my preliminary assumption was incorrect. Nevertheless, as I stated, this function is still valuable and complementary to StartImmediate, as it ensures, it starts a task immediately and when done, it returns the task itself. Though not sure what I should do to complete this pull request. |
@OnurGumus The PR is ready - you don't have to do anything more. It's just a question of when we're willing to rev FSharp.Core (and if we rev the version number etc.) |
Feature implemented.