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

SynchronizationContext isn't properly restored on async call #1593

Closed
hlngo opened this Issue Jun 6, 2017 · 12 comments

Comments

Projects
None yet
3 participants
@hlngo

hlngo commented Jun 6, 2017

The first OpenAsync() call results in Thread.CurrentThread switching from thread id 1 to another random thread. The subsequent calls to OpenAsync() do not cause this switch.

The consequence of the thread switch is that this may cause cross-thread operation. See code below.

private async void button1_Click(object sender, EventArgs e)
{
var strBuilder = new Npgsql.NpgsqlConnectionStringBuilder()
{
Host = "localhost",
Username = "postgres",
Password = "password"
};
using (var conn = new Npgsql.NpgsqlConnection(strBuilder.ConnectionString))
{
try
{
await conn.OpenAsync();
if (conn.State ==ConnectionState.Open)
{
MessageBox.Show("Connected");
this.button1.Text = "CROSS-THREAD";
}
}
catch (Exception ex)
{
MessageBox.Show(ex.Message);
}
}
}

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 6, 2017

Member

The behavior you're describing is expected, and indeed all asynchronous operations function in this way. When you await an async method, you're accepting the fact that the lines of code following it (the continuation) may (but also might not) be executed in a different thread. In your specific example, the first time you open the connection, a physical connection is opened and so actual I/O occurs and a thread switch occurs. The second and thread time the connection pool probably returns the idle physical connection, meaning that no I/O actually occurs.

In cases where thread switching may be an issue, e.g. UI frameworks where changes must be done by a single UI thread, the SynchronizationContext is there to make sure that continuations are scheduled for work on the proper thread. There are many posts out there describing how SynchronizationContext interacts with async/await in the context of Winforms/WPF to make sure that code is executed on the right thread.

I'm going to close this as Npgsql is behaving as expected, but if you think otherwise (or need to understand this better) don't hesitate to post back here.

Member

roji commented Jun 6, 2017

The behavior you're describing is expected, and indeed all asynchronous operations function in this way. When you await an async method, you're accepting the fact that the lines of code following it (the continuation) may (but also might not) be executed in a different thread. In your specific example, the first time you open the connection, a physical connection is opened and so actual I/O occurs and a thread switch occurs. The second and thread time the connection pool probably returns the idle physical connection, meaning that no I/O actually occurs.

In cases where thread switching may be an issue, e.g. UI frameworks where changes must be done by a single UI thread, the SynchronizationContext is there to make sure that continuations are scheduled for work on the proper thread. There are many posts out there describing how SynchronizationContext interacts with async/await in the context of Winforms/WPF to make sure that code is executed on the right thread.

I'm going to close this as Npgsql is behaving as expected, but if you think otherwise (or need to understand this better) don't hesitate to post back here.

@roji roji closed this Jun 6, 2017

@roji roji added the invalid label Jun 6, 2017

@hlngo

This comment has been minimized.

Show comment
Hide comment
@hlngo

hlngo Jun 6, 2017

Roji, thanks for detailed explanation. I got this issue and went back to test with sqlserver client, mysql connector, sqlite and none of them has this issue. I read another issue #121 (#121) in this repos and you guys talking something about using true async. Not sure if it is related. My point here is this issue doesn't happen with other connectors while it always happens with npgsql.

hlngo commented Jun 6, 2017

Roji, thanks for detailed explanation. I got this issue and went back to test with sqlserver client, mysql connector, sqlite and none of them has this issue. I read another issue #121 (#121) in this repos and you guys talking something about using true async. Not sure if it is related. My point here is this issue doesn't happen with other connectors while it always happens with npgsql.

@hlngo

This comment has been minimized.

Show comment
Hide comment
@hlngo

hlngo Jun 7, 2017

Roji, I have read some more about what you explained. I found an interesting article on msdn: https://blogs.msdn.microsoft.com/pfxteam/2012/01/20/await-synchronizationcontext-and-console-apps/
It describes how await “brings you back to where you were.” I read similar things on other sites as well. I think it would be great if Npgsql can do that.

hlngo commented Jun 7, 2017

Roji, I have read some more about what you explained. I found an interesting article on msdn: https://blogs.msdn.microsoft.com/pfxteam/2012/01/20/await-synchronizationcontext-and-console-apps/
It describes how await “brings you back to where you were.” I read similar things on other sites as well. I think it would be great if Npgsql can do that.

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 7, 2017

Member

To make things clear, in normal sync I/O you thread blocks (sits around idly) while waiting for server responses. This is why you have the same thread before and after calling the synchronous Open(). The idea behind async is that the thread is released while waiting for the server response (which could take a long time), and that another thread picks up running the code after OpenAsync(). So, if you're doing await on any async operation and see the same thread ID (as seen via Thread.CurrentThread.ManagedThreadId), either that operation isn't actually implemented as async, or it had cached resources that allowed it to skip actual I/O for this specific invocation.

If memory serves, last time I checked the standard MySQL ADO.NET provider indeed did not implement async (it also supported old versions of the .NET Framework, from before async/await was introduced). On the other hand, I'm 99% sure that SqlClient fully supports async, so you may want to go back and check things.

Finally, the article you find explains the situation well: the point of SynchronizationContext is indeed to "bring you back to where you were", or in other words, to make sure the code after your await runs in the UI thread. However, this has nothing to do with Npgsql, it's a feature of the code calling Npgsql's OpenAsync(). Within Npgsql the SynchronizationContext is ignored as it should be (via an equivalent of ConfigureAwait(false), since code inside Npgsql does not rely on the UI thread. Your own function, awaiting on OpenAsync(), should have its SynchronizationContext properly set, and that is what will make your code resume back in the UI thread after Npgsql completes its own part.

To better understand these concepts, you may want to play around with a simper async API, for example asynchronously opening a file or socket somewhere. You should see the same effects as with Npgsql.

Member

roji commented Jun 7, 2017

To make things clear, in normal sync I/O you thread blocks (sits around idly) while waiting for server responses. This is why you have the same thread before and after calling the synchronous Open(). The idea behind async is that the thread is released while waiting for the server response (which could take a long time), and that another thread picks up running the code after OpenAsync(). So, if you're doing await on any async operation and see the same thread ID (as seen via Thread.CurrentThread.ManagedThreadId), either that operation isn't actually implemented as async, or it had cached resources that allowed it to skip actual I/O for this specific invocation.

If memory serves, last time I checked the standard MySQL ADO.NET provider indeed did not implement async (it also supported old versions of the .NET Framework, from before async/await was introduced). On the other hand, I'm 99% sure that SqlClient fully supports async, so you may want to go back and check things.

Finally, the article you find explains the situation well: the point of SynchronizationContext is indeed to "bring you back to where you were", or in other words, to make sure the code after your await runs in the UI thread. However, this has nothing to do with Npgsql, it's a feature of the code calling Npgsql's OpenAsync(). Within Npgsql the SynchronizationContext is ignored as it should be (via an equivalent of ConfigureAwait(false), since code inside Npgsql does not rely on the UI thread. Your own function, awaiting on OpenAsync(), should have its SynchronizationContext properly set, and that is what will make your code resume back in the UI thread after Npgsql completes its own part.

To better understand these concepts, you may want to play around with a simper async API, for example asynchronously opening a file or socket somewhere. You should see the same effects as with Npgsql.

@hlngo

This comment has been minimized.

Show comment
Hide comment
@hlngo

hlngo Jun 7, 2017

Thanks Roji. I have got a bit clearer after reading your explanation and Stephen Cleary's article:
https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

However, I may need some diggings on why I don't see the same thing happening in other drivers. FYI, I was talking about the other MySQL: https://github.com/mysql-net/MySqlConnector

hlngo commented Jun 7, 2017

Thanks Roji. I have got a bit clearer after reading your explanation and Stephen Cleary's article:
https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

However, I may need some diggings on why I don't see the same thing happening in other drivers. FYI, I was talking about the other MySQL: https://github.com/mysql-net/MySqlConnector

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 7, 2017

Member

@hlngo yeah, Stephen Cleary has some other very useful articles on async/await, make sure you understand exactly what ConfigureAwait() does and how it interacts with SynchronizationContext.

As to other providers, I really can't say... But you should see thread-switching everywhere where you await on a truly async operation which performs I/O, unless the SynchronizationContext is making your continuation run back on your UI thread (but then this should happen when using Npgsql as well).

Good luck and let me know if you need more help (or find more info on other drivers).

Member

roji commented Jun 7, 2017

@hlngo yeah, Stephen Cleary has some other very useful articles on async/await, make sure you understand exactly what ConfigureAwait() does and how it interacts with SynchronizationContext.

As to other providers, I really can't say... But you should see thread-switching everywhere where you await on a truly async operation which performs I/O, unless the SynchronizationContext is making your continuation run back on your UI thread (but then this should happen when using Npgsql as well).

Good luck and let me know if you need more help (or find more info on other drivers).

@hlngo

This comment has been minimized.

Show comment
Hide comment
@hlngo

hlngo Jun 7, 2017

@roji I think I got it why npgsql behaves like that. Npgsql sets the current synchronization context to null instead of use ConfigureAwait(false). See link:
https://github.com/npgsql/npgsql/blob/2dd46e7c544caf3302ca7b89dd888a16dccf5c2c/src/Npgsql/PGUtil.cs
When using ConfigureAwait(false), after leaving the OpenAsync(), the context will switch back to the previous context, Winforms context in this case.
I also tried ReadAsync a big file as well as query big data in SQL Server, both of them always bring me back to the UI thread.
Another quick search on other driver repos is that they don't touch the SynchronizationContext.
Also, I have posted a question on StackOverflow to get more info from other people:
https://stackoverflow.com/questions/44418761/cross-thread-exception-after-async-call
Thanks again for your help and big thanks for your work on npgsql.

hlngo commented Jun 7, 2017

@roji I think I got it why npgsql behaves like that. Npgsql sets the current synchronization context to null instead of use ConfigureAwait(false). See link:
https://github.com/npgsql/npgsql/blob/2dd46e7c544caf3302ca7b89dd888a16dccf5c2c/src/Npgsql/PGUtil.cs
When using ConfigureAwait(false), after leaving the OpenAsync(), the context will switch back to the previous context, Winforms context in this case.
I also tried ReadAsync a big file as well as query big data in SQL Server, both of them always bring me back to the UI thread.
Another quick search on other driver repos is that they don't touch the SynchronizationContext.
Also, I have posted a question on StackOverflow to get more info from other people:
https://stackoverflow.com/questions/44418761/cross-thread-exception-after-async-call
Thanks again for your help and big thanks for your work on npgsql.

@hlngo

This comment has been minimized.

Show comment
Hide comment
@hlngo

hlngo Jun 8, 2017

@roji Ok. I confirm that changing from ConfigureAwait(false) to NoSynchronizationContextScope causing the effect. I found the commit below on Sep 24 last year:
366b4bf

Then I went back to install 3.1.7, which is the release right before the commit above, and I don't see the cross-thread exception anymore. The thread id stays the same before and after calling OpenAsync(). I will post my findings to stackoverflow. It may help other people.

What do you think about this?

hlngo commented Jun 8, 2017

@roji Ok. I confirm that changing from ConfigureAwait(false) to NoSynchronizationContextScope causing the effect. I found the commit below on Sep 24 last year:
366b4bf

Then I went back to install 3.1.7, which is the release right before the commit above, and I don't see the cross-thread exception anymore. The thread id stays the same before and after calling OpenAsync(). I will post my findings to stackoverflow. It may help other people.

What do you think about this?

@StephenCleary

This comment has been minimized.

Show comment
Hide comment
@StephenCleary

StephenCleary Jun 8, 2017

I believe the NoSynchronizationContextScope (as it is currently used) is buggy. The scope must be disposed synchronously, before the method returns an incomplete task.

StephenCleary commented Jun 8, 2017

I believe the NoSynchronizationContextScope (as it is currently used) is buggy. The scope must be disposed synchronously, before the method returns an incomplete task.

@roji roji added 🐞 bug and removed invalid labels Jun 8, 2017

@roji roji added this to the 3.2.4 milestone Jun 8, 2017

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 8, 2017

Member

@StephenCleary, reading up on the stackoverflow thread I understand the issue. @hlngo, thanks for being insistent on this issue and diving into it. I'll switch to @StephenCleary's SynchronizationContextSwitcher for 3.2.4.

Member

roji commented Jun 8, 2017

@StephenCleary, reading up on the stackoverflow thread I understand the issue. @hlngo, thanks for being insistent on this issue and diving into it. I'll switch to @StephenCleary's SynchronizationContextSwitcher for 3.2.4.

@roji roji reopened this Jun 8, 2017

@hlngo

This comment has been minimized.

Show comment
Hide comment
@hlngo

hlngo Jun 8, 2017

@roji @StephenCleary Thank you both for your work and explanation. I have learnt a lot from this discussion.

hlngo commented Jun 8, 2017

@roji @StephenCleary Thank you both for your work and explanation. I have learnt a lot from this discussion.

@roji roji changed the title from Thread switch after the very first OpenAsync() is called to SynchronizationContext isn't properly restored on async call Jun 11, 2017

@roji roji closed this in c0ca865 Jun 11, 2017

roji added a commit that referenced this issue Jun 11, 2017

Fix sync context switching
Our previous SynchronizationContext switching mechanism was bad.
Thanks @StephenCleary and @hlngo

Fixes #1593

(cherry picked from commit c0ca865)
@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Jun 12, 2017

Member

Note: this has been fixed. You can test the new synchronization context switching mechanism (adapted from @StephenCleary's library) by using versions starting from 3.2.4-ci-00189 in the stable myget feed.

Member

roji commented Jun 12, 2017

Note: this has been fixed. You can test the new synchronization context switching mechanism (adapted from @StephenCleary's library) by using versions starting from 3.2.4-ci-00189 in the stable myget feed.

roji added a commit to roji/Npgsql that referenced this issue Oct 25, 2017

Better resetting of sync context
Stop using the delegate-based SynchronizationContextSwitcher, switching
back to NoSynchronizationContextScope but calling it only from sync
method wrappers of async methods. This avoids the heap allocation
associated with the delegate.

Also switched to using the allocation-free overload of
CancellationToken.Register().

Based on #1688 (thanks @anpete) but still avoids #1593.

roji added a commit to roji/Npgsql that referenced this issue Oct 25, 2017

Better resetting of sync context
Stop using the delegate-based SynchronizationContextSwitcher, switching
back to NoSynchronizationContextScope but calling it only from sync
method wrappers of async methods. This avoids the heap allocation
associated with the delegate.

Also switched to using the allocation-free overload of
CancellationToken.Register().

Based on #1688 (thanks @anpete) but still avoids #1593.

roji added a commit to roji/Npgsql that referenced this issue Oct 25, 2017

Better resetting of sync context
Stop using the delegate-based SynchronizationContextSwitcher, switching
back to NoSynchronizationContextScope but calling it only from sync
method wrappers of async methods. This avoids the heap allocation
associated with the delegate.

Also switched to using the allocation-free overload of
CancellationToken.Register().

Based on #1688 (thanks @anpete) but still avoids #1593.

roji added a commit that referenced this issue Oct 29, 2017

Better resetting of sync context
Stop using the delegate-based SynchronizationContextSwitcher, switching
back to NoSynchronizationContextScope but calling it only from sync
method wrappers of async methods. This avoids the heap allocation
associated with the delegate.

Also switched to using the allocation-free overload of
CancellationToken.Register().

Based on #1688 (thanks @anpete) but still avoids #1593.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment