SaveChangesAsync doesn't preserve the sync context across retries

[roji]

In this Helix build we have the following test failure, seems to be flaky:

    Microsoft.EntityFrameworkCore.Query.QueryBugsTest.SaveChangesAsync_accepts_changes_with_ConfigureAwait_true_22841 [FAIL]
      Assert.True() Failure
      Expected: True
      Actual:   False
      Stack Trace:
        D:\a\_work\1\s\test\EFCore.SqlServer.FunctionalTests\Query\QueryBugsTest.cs(9132,0): at Microsoft.EntityFrameworkCore.Query.QueryBugsTest.SaveChangesAsync_accepts_changes_with_ConfigureAwait_true_22841()
        --- End of stack trace from previous location ---

[ajcvickers]

@roji Disable the test for now?

[roji]

I think we've only seen it fail like once (AFAIK), will try to take a look at it in the next few days, if it fails again we can disable for sure...

[ajcvickers]

@roji I am probably missing something, but it looks to me like SaveChangeAsync always uses ConfigureAwait(false).

var entitiesSaved = interceptionResult.HasResult
    ? interceptionResult.Result
    : await DbContextDependencies.StateManager
        .SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken)
        .ConfigureAwait(false);

efcore/src/EFCore/DbContext.cs

Line 730 in 197479e

var entitiesSaved = interceptionResult.HasResult

[roji]

@ajcvickers as discussed offline, ConfigureAwait controls where the continuation runs, i.e. the code after SaveChangesAsync - and we don't really care about that.

[vonzshik]

@roji from the looks of it, SingleThreadSynchronizationContext22841 has the same issue SingleThreadSynchronizationContext had in npgsql (npgsql/npgsql/issues/3856).

[roji]

@vonzshik you're right - I just discovered that on my own; though that's just a test issue (note also that the test asserts on the current thread, rather than on the current sync context).

Here's what I think is going on... Under normal circumstances, everything does work just fine; before AcceptAllChanges is called (the method that jumps into user code), all code is either synchronous, or asynchronous with ConfigureAwait(true).

However, if we retry because of an actual error, then the 2nd attempt by the execution strategy will have lost the synchronization context, since the 1st attempt will have been called with ConfigureAwait(false). This would also explain the flakiness of the test (as opposed to it always passing/failing) - we indeed have occasional errors when running our tests in the CI environment, so I'm thinking that in those cases the test failed.

[ajcvickers]

@roji SaveChangesAsync_accepts_changes_with_ConfigureAwait_true_22841 just failed on my C.I. run.

#26996

[smitpatel]

Also failed in #27011

[vonzshik]

@roji what if the problem here is in Thread.CurrentThread == trackingSynchronizationContext.Thread check? That is, while an async version of SaveChangesAsync is called, there is no guarantee that at any point it's actually going to yield, so it's not going to execute a continuation on that thread?

If so, calling await Task.Yield() just after the context is set should help.

[roji]

@vonzshik that's exactly what #27019 is supposed to address (I did it just 4 hours before your comment 😉).

However, I can't see how SaveChangesAsync wouldn't yield... it's guaranteed to insert a row into SQL Server, which is supposed to always do networking and yield... If there's some situation in which SqlClient does sync I/O instead of async, then that would explain it; I'm not aware of such a thing (and it shouldn't happen), but it's possible.

[ajcvickers]

@roji It's not doing "networking" when using LocalDb. It's doing named-pipes.

[roji]

AFAIK that's still I/O that's supposed to be async... Put another way, if LocalDB communication were always sync, this test would have failed consistently, rather than being flaky. But maybe under some conditions SqlClient decides to do sync I/O...

[vonzshik]

AFAIK that's still I/O that's supposed to be async... Put another way, if LocalDB communication were always sync, this test would have failed consistently, rather than being flaky. But maybe under some conditions SqlClient decides to do sync I/O...

Well, for example, SqlClient's login handshake is sync only (because it's called from a constructor)

https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs#L513

[roji]

😕

I'd still hope that ExecuteReaderAsync (as opposed to logging in) would actually be async - but you're right that we can't be sure...

[roji]

Keeping open for a few days to make sure we re-discuss in triage and see if the test failed after #27019.