Closed benwulfe closed 6 years ago
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 notifications@github.com 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 https://github.com/ParsePlatform/Parse-SDK-dotNET/pull/251#issuecomment-277823456, 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 Parse apis query and save still work as expected in a test unity3d project. ContinueWith still properly marshals
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: https://stackoverflow.com/questions/24441501/main-thread-issue-with-parse-com-queries This is not related to this change.
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. The main problem is that since Parse compiles all source together into Parse.Unity.dll, a developer who wishes to use both Parse and Firebase (say for Firebase Cloud Messaging) must use the Parse version of Task. If this version in Parse is not fixed, then Firebase may not function properly. Regards
@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.
Merging #251 into master will not change coverage. The diff coverage is
n/a
.
@@ 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 198
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 6d50fab...663ee2c. Read the comment docs.
@benwulfe sorry for the delay! We're actively working on changes atm, expect to see some new stuff soon.
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