Task.Run should run tasks on the threadpool #251
Conversation
…nchronizationContext.Current. See: http://stackoverflow.com/questions/41959490/firebase-makes-tasks-execute-only-on-main-thread-causing-unity-to-freeze/41988539#41988539 Basically what's going on is that the TaskFactory defaults to the SynchronizationContext.Current scheduler, which is not correct as new tasks always should be run on the threadpool. TaskScheduler.FromCurrentSynchronizationContext() is instead intended to be used with ContinueWith as a mechanism for controlling the thread that the callback will run on. But adding that support is a new feature. Supporting information: MSDN Documentation that Task.Run should run on the threadpool: https://www.google.com/url?q=https://msdn.microsoft.com/en-us/library/system.threading.tasks.task.run(v%3Dvs.110).aspx&sa=D&usg=AFQjCNFx3qW3vSsmhpDONsRDBdynzbKHgA source code for mono that defaults to threadpool: https://www.google.com/url?q=https://github.com/mono/mono/blob/0bcbe39b148bb498742fc68416f8293ccd350fb6/mcs/class/referencesource/mscorlib/system/threading/Tasks/TaskFactory.cs%23L93&sa=D&usg=AFQjCNGLQ0gm5KlaitmEV2xr29BHutDKNQ
|
It's been a REALLY long time since I dealt with this code, but IIRC (and maybe @hallucinogen can remember), we had it this way to support Unity's strange thread scheduling with it's WWW class, which could only be used from the main thread. If all the relevant async APIs still work with this change (and you can show a quick demo of the APIs working, log output is more than enough), I'm more than happy to change this to be more accurate toward the standard .NET version. |
|
Thanks for looking at it. are you talking Task api's specifically? I can
do that.
Note that its quite rare for SynchronizationContext.Current to be set in
Unity. And the default behavior when its not set is to do the equivalent
of what I am doing here.
…On Mon, Feb 6, 2017 at 1:43 PM, Richard Ross ***@***.***> wrote:
It's been a REALLY long time since I dealt with this code, but IIRC (and
maybe @hallucinogen <https://github.com/hallucinogen> can remember), we
had it this way to support Unity's strange thread scheduling with it's WWW
class, which could only be used from the main thread.
If all the relevant async APIs still work with this change (and you can
show a quick demo of the APIs working, log output is more than enough), I'm
more than happy to change this to be more accurate toward the standard .NET
version.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#251 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AH7cpeUmpDGDAbgTW2i5UAWHzH4Gp-C1ks5rZ5QQgaJpZM4L0gNJ>
.
|
|
I'm referring to the Parse APIs here (ParseObject.fetch, etc.). As long as those still function, this is a solid change. |
|
It's been quite a while, but I've finally come back to this, got a local parse server working along with the unity sdk rebuilt and working in a test app. All unit tests pass (192/192) with this change in place Really, this change should not be impactful in most scenarios. You would only see a change in behavior if SynchronizationContext.Current was set -- which it is not in most situations in unity3d. Another interesting fact is that without SynchronizationContext being set, the ContinueWith callback will always be executed on a worker thread which you can see evidence of user issues such as this one here: Please let me know if there is any additional testing you'd like me to do. |
|
Note that this is an important thing to get in for Google Firebase (I work for Google) which uses the Task library built here. |
|
@benwulfe glad to see you've managed to get back to this! I've been meaning to put some TLC into this project, but I'm going to be gone for a couple weeks coming up soon here. I don't think I'll have a chance to look at this until I'm back, but once I am I'll double check those tests and see how things look. |
|
ping! |
|
@benwulfe whoops time seems to fly! I'll take some time to look over the tests tomorrow and get back to you in the afternoon, apologies. |
montymxb
left a comment
montymxb
left a comment
There was a problem hiding this comment.
Alright changes look good to me! @flovilmart this should be good to go. There will be more coming up before we do another release, but this is getting in that direction.
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
======================================
Coverage 67.2% 67.2%
======================================
Files 127 127
Lines 10139 10139
Branches 1455 1455
======================================
Hits 6814 6814
Misses 3127 3127
Partials 198 198Continue to review full report at Codecov.
|
|
@benwulfe sorry for the delay! We're actively working on changes atm, expect to see some new stuff soon. |
4 participants
Task.Run should run tasks on the threadpool but are instead run on SynchronizationContext.Current.
See: http://stackoverflow.com/questions/41959490/firebase-makes-tasks-execute-only-on-main-thread-causing-unity-to-freeze/41988539#41988539
Basically what's going on is that the TaskFactory defaults to the SynchronizationContext.Current scheduler, which is not correct as new tasks always should be run on the threadpool. TaskScheduler.FromCurrentSynchronizationContext() is instead intended to be used with ContinueWith as a mechanism for controlling the thread that the callback will run on. But adding that support is a new feature.
Supporting information:
MSDN Documentation that Task.Run should run on the threadpool:
https://www.google.com/url?q=https://msdn.microsoft.com/en-us/library/system.threading.tasks.task.run(v%3Dvs.110).aspx&sa=D&usg=AFQjCNFx3qW3vSsmhpDONsRDBdynzbKHgA
source code for mono that defaults to threadpool:
https://www.google.com/url?q=https://github.com/mono/mono/blob/0bcbe39b148bb498742fc68416f8293ccd350fb6/mcs/class/referencesource/mscorlib/system/threading/Tasks/TaskFactory.cs%23L93&sa=D&usg=AFQjCNGLQ0gm5KlaitmEV2xr29BHutDKNQ