Skip to content
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

Stream.ReadAsync continues on different thread #16759

Closed
mrward opened this issue Sep 10, 2019 · 7 comments · Fixed by #16945
Closed

Stream.ReadAsync continues on different thread #16759

mrward opened this issue Sep 10, 2019 · 7 comments · Fixed by #16945

Comments

@mrward
Copy link
Member

@mrward mrward commented Sep 10, 2019

Steps to Reproduce

  1. Use code: await stream.ReadAsync (buf, 0, count);

The above should continue on the same thread that called ReadAsync but seems to continue on a different thread.

awaiting a Task.Run works as expected.

Sample Xamarin.Mac project:

https://github.com/mrward/TestStreamReadAsync

Slightly more complicated console app with a custom synchronisation context.

https://github.com/mrward/TestStreamReadAsyncConsoleApp

Also affects VS Mac.

Current Behavior

After the await the thread has changed.
awaiting a Task.Run works as expected.

Expected Behavior

After the await the thread is the same as the one that called ReadAsync.

On which platforms did you notice this

[x] macOS
[ ] Linux
[ ] Windows

Version Used:

Mono JIT compiler version 6.4.0.188 (2019-06/2c3aeaf3780 Tue Sep 3 15:10:54 EDT 2019)
Mono JIT compiler version 6.0.0.327 (2019-02/f8ea05bddcb Sat Aug 10 18:47:12 EDT 2019)

@slluis

This comment has been minimized.

Copy link
Member

@slluis slluis commented Sep 10, 2019

This is causing https://devdiv.visualstudio.com/DevDiv/_workitems/edit/978857, and who know what else.

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Sep 10, 2019

What SynchronizationContext is this with?

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Sep 10, 2019

@slluis

This comment has been minimized.

Copy link
Member

@slluis slluis commented Sep 10, 2019

In the case of VSMac, the sync context is GtkSynchronizationContext (http://source.monodevelop.com/#MonoDevelop.Ide/MonoDevelop.Ide/DispatchService.cs,61154603aec2972a). The context is correctly set before the await, and restored after the call, but the continuation is not dispatched through the context.

@steveisok

This comment has been minimized.

Copy link
Contributor

@steveisok steveisok commented Sep 10, 2019

I can reproduce it through a console app with a single threaded sync context. It seems like the SyncContext Post is never called during await stream.ReadAsync(buf, 0, count)

@steveisok

This comment has been minimized.

Copy link
Contributor

@steveisok steveisok commented Sep 10, 2019

I think it may have to do w/ our FileStream implementation. If I use a FileStream with async set to true, then it'll work as expected.

new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read, 1024, true)

If you use File.OpenRead, async will be false.

@marek-safar marek-safar added this to the 2019-06 (6.4.xx) milestone Sep 17, 2019
@steveisok steveisok self-assigned this Sep 19, 2019
@steveisok

This comment has been minimized.

Copy link
Contributor

@steveisok steveisok commented Sep 19, 2019

I did some digging and our fix to this issue #12421 caused problems.

In this change, we made the sync context always be a reference:

if (0 == (options & CaptureOptions.IgnoreSyncCtx))
#if MONO
syncCtxNew = ecCurrent.SynchronizationContext;

I believe this caused the continuation to be executed on the same thread as the spawned task because of:

https://github.com/mono/corert/blob/ffcd7990d87e560c63413293c8efe1d1fc4e3584/src/System.Private.CoreLib/src/System/Threading/Tasks/TaskContinuation.cs#L403-L407

Note the check m_syncContext == SynchronizationContext.Current)

If our executioncontext change is reverted, then a new sync context will be created and bound to a new task. This allows the continuation to be posted to the sync context and in this case, will ensure the continuation runs on the UI thread.

PR forthcoming.

steveisok pushed a commit to steveisok/mono that referenced this issue Sep 19, 2019
copying a new SynchronizationContext as opposed to a reference to the
existing one.

The old behavior caused SynchronizationContextAwaitTaskContinuations to not
post to a SynchronizationContext.

Fixes mono#16759
steveisok added a commit that referenced this issue Sep 23, 2019
Restores previous ExecutionContext Capture behavior where we were copying a new SynchronizationContext as opposed to a reference to the existing one.

The old behavior caused SynchronizationContextAwaitTaskContinuations to not post to a SynchronizationContext.

Fixes #16759

Reverts a fix for #12421
monojenkins added a commit to monojenkins/mono that referenced this issue Sep 23, 2019
…ying a new SynchronizationContext as opposed to a reference to the existing one.

The old behavior caused SynchronizationContextAwaitTaskContinuations to not
post to a SynchronizationContext.

Fixes mono#16759
monojenkins added a commit to monojenkins/mono that referenced this issue Sep 23, 2019
…ying a new SynchronizationContext as opposed to a reference to the existing one.

The old behavior caused SynchronizationContextAwaitTaskContinuations to not
post to a SynchronizationContext.

Fixes mono#16759
steveisok added a commit that referenced this issue Sep 23, 2019
Restores previous ExecutionContext Capture behavior where we were copying a new SynchronizationContext as opposed to a reference to the existing one.

The old behavior caused SynchronizationContextAwaitTaskContinuations to not post to a SynchronizationContext.

Fixes #16759

Reverts a fix for #12421

Backport of #16945
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Sep 24, 2019
Changes: mono/mono@bfbf823...6d40f13

Context: mono/mono#15646
Context: mono/mono#16689
Context: mono/mono#16759
Context: mono/mono#16824
Context: mono/mono#16918
Context: mono/mono#16974

Improve exception filtering within the debugger.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Sep 25, 2019
Changes: mono/mono@bfbf823...6d40f13

Context: mono/mono#15646
Context: mono/mono#16689
Context: mono/mono#16759
Context: mono/mono#16824
Context: mono/mono#16918
Context: mono/mono#16974

Improve exception filtering within the debugger.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Sep 27, 2019
Changes: mono/mono@bfbf823...6d40f13

Context: mono/mono#15646
Context: mono/mono#16689
Context: mono/mono#16759
Context: mono/mono#16824
Context: mono/mono#16918
Context: mono/mono#16974

Improve exception filtering within the debugger.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Dec 3, 2019
Changes: mono/api-snapshot@fc50bc4...45a61d9

        $ git diff --shortstat fc50bc4f...45a61d93
         22 files changed, 775 insertions(+), 474 deletions(-)

Changes: mono/cecil@a6c8f5e...a6a7f5c

        $ git diff --shortstat a6c8f5e1...a6a7f5c0
         55 files changed, 818 insertions(+), 530 deletions(-)

Changes: mono/corefx@1f87de3...49f1c45

        $ git diff --shortstat e4f7102b...49f1c453
         38 files changed, 1171 insertions(+), 419 deletions(-)

Changes: mono/linker@ebe2a1f...e8d054b

        $ git diff --shortstat ebe2a1f4...e8d054bf
         137 files changed, 5360 insertions(+), 1781 deletions(-)

Changes: mono/mono@8946e49...18920a8

        $ git diff --shortstat 8946e49a...18920a83
         1811 files changed, 47240 insertions(+), 48331 deletions(-)

Changes: xamarin/xamarin-android-api-compatibility@a61271e...50a3c52

        $ git diff --shortstat a61271e0...50a3c52d
         1 file changed, 2 insertions(+), 791 deletions(-)

Fixes: #3619

Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1005448
Context: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/967582
Context: dotnet/coreclr#26370
Context: dotnet/coreclr#26479
Context: dotnet/corefx#40455
Context: dotnet/corefx#40578
Context: mono/mono#7377
Context: mono/mono#12421
Context: mono/mono#12586
Context: mono/mono#14080
Context: mono/mono#14725
Context: mono/mono#14772
Context: mono/mono#15261
Context: mono/mono#15262
Context: mono/mono#15263
Context: mono/mono#15307
Context: mono/mono#15308
Context: mono/mono#15310
Context: mono/mono#15646
Context: mono/mono#15687
Context: mono/mono#15805
Context: mono/mono#15992
Context: mono/mono#15994
Context: mono/mono#15999
Context: mono/mono#16032
Context: mono/mono#16034
Context: mono/mono#16046
Context: mono/mono#16192
Context: mono/mono#16308
Context: mono/mono#16310
Context: mono/mono#16369
Context: mono/mono#16380
Context: mono/mono#16381
Context: mono/mono#16395
Context: mono/mono#16411
Context: mono/mono#16415
Context: mono/mono#16486
Context: mono/mono#16570
Context: mono/mono#16605
Context: mono/mono#16616
Context: mono/mono#16689
Context: mono/mono#16701
Context: mono/mono#16712
Context: mono/mono#16742
Context: mono/mono#16759
Context: mono/mono#16803
Context: mono/mono#16808
Context: mono/mono#16824
Context: mono/mono#16876
Context: mono/mono#16879
Context: mono/mono#16918
Context: mono/mono#16943
Context: mono/mono#16950
Context: mono/mono#16974
Context: mono/mono#17004
Context: mono/mono#17017
Context: mono/mono#17038
Context: mono/mono#17040
Context: mono/mono#17083
Context: mono/mono#17084
Context: mono/mono#17133
Context: mono/mono#17139
Context: mono/mono#17151
Context: mono/mono#17180
Context: mono/mono#17278
Context: mono/mono#17549
Context: mono/mono#17569
Context: mono/mono#17665
Context: mono/mono#17687
Context: mono/mono#17737
Context: mono/mono#17790
Context: mono/mono#17924
Context: mono/mono#17931
Context: https://github.com/mono/mono/issues/26758
Context: https://github.com/mono/mono/issues/37913
Context: xamarin/xamarin-macios#7005
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Dec 3, 2019
Changes: mono/api-snapshot@fc50bc4...45a61d9

        $ git diff --shortstat fc50bc4f...45a61d93
         22 files changed, 775 insertions(+), 474 deletions(-)

Changes: mono/cecil@a6c8f5e...a6a7f5c

        $ git diff --shortstat a6c8f5e1...a6a7f5c0
         55 files changed, 818 insertions(+), 530 deletions(-)

Changes: mono/corefx@1f87de3...49f1c45

        $ git diff --shortstat e4f7102b...49f1c453
         38 files changed, 1171 insertions(+), 419 deletions(-)

Changes: mono/linker@ebe2a1f...e8d054b

        $ git diff --shortstat ebe2a1f4...e8d054bf
         137 files changed, 5360 insertions(+), 1781 deletions(-)

Changes: mono/mono@8946e49...18920a8

        $ git diff --shortstat 8946e49a...18920a83
         1811 files changed, 47240 insertions(+), 48331 deletions(-)

Changes: xamarin/xamarin-android-api-compatibility@a61271e...50a3c52

        $ git diff --shortstat a61271e0...50a3c52d
         1 file changed, 2 insertions(+), 791 deletions(-)

Fixes: #3619

Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1005448
Context: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/967582
Context: dotnet/coreclr#26370
Context: dotnet/coreclr#26479
Context: dotnet/corefx#40455
Context: dotnet/corefx#40578
Context: mono/mono#7377
Context: mono/mono#12421
Context: mono/mono#12586
Context: mono/mono#14080
Context: mono/mono#14725
Context: mono/mono#14772
Context: mono/mono#15261
Context: mono/mono#15262
Context: mono/mono#15263
Context: mono/mono#15307
Context: mono/mono#15308
Context: mono/mono#15310
Context: mono/mono#15646
Context: mono/mono#15687
Context: mono/mono#15805
Context: mono/mono#15992
Context: mono/mono#15994
Context: mono/mono#15999
Context: mono/mono#16032
Context: mono/mono#16034
Context: mono/mono#16046
Context: mono/mono#16192
Context: mono/mono#16308
Context: mono/mono#16310
Context: mono/mono#16369
Context: mono/mono#16380
Context: mono/mono#16381
Context: mono/mono#16395
Context: mono/mono#16411
Context: mono/mono#16415
Context: mono/mono#16486
Context: mono/mono#16570
Context: mono/mono#16605
Context: mono/mono#16616
Context: mono/mono#16689
Context: mono/mono#16701
Context: mono/mono#16712
Context: mono/mono#16742
Context: mono/mono#16759
Context: mono/mono#16803
Context: mono/mono#16808
Context: mono/mono#16824
Context: mono/mono#16876
Context: mono/mono#16879
Context: mono/mono#16918
Context: mono/mono#16943
Context: mono/mono#16950
Context: mono/mono#16974
Context: mono/mono#17004
Context: mono/mono#17017
Context: mono/mono#17038
Context: mono/mono#17040
Context: mono/mono#17083
Context: mono/mono#17084
Context: mono/mono#17133
Context: mono/mono#17139
Context: mono/mono#17151
Context: mono/mono#17180
Context: mono/mono#17278
Context: mono/mono#17549
Context: mono/mono#17569
Context: mono/mono#17665
Context: mono/mono#17687
Context: mono/mono#17737
Context: mono/mono#17790
Context: mono/mono#17924
Context: mono/mono#17931
Context: https://github.com/mono/mono/issues/26758
Context: https://github.com/mono/mono/issues/37913
Context: xamarin/xamarin-macios#7005
@Therzok Therzok added this to Needs triage in VS4M Tracker via automation Dec 23, 2019
@Therzok Therzok moved this from Needs triage to Closed in VS4M Tracker Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
VS4M Tracker
  
Closed
4 participants
You can’t perform that action at this time.