fix(csharp/src/Client): stop blocking on async calls in the ADO.NET wrapper - #4716
Merged
Conversation
…rapper AdbcCommand overrode only ExecuteDbDataReader and AdbcDataReader only Read(), so both BCL async entry points fell back to the synchronous bodies and reached .Result. One of those sits inside a private method named ReadNextRecordBatchAsync that returns ValueTask<RecordBatch?> and takes a CancellationToken it never used. On a host with a SynchronizationContext the awaited path deadlocks. ReadNextRecordBatchAsync now awaits the stream. AdbcDataReader overrides ReadAsync, keeping the intra-batch path free of a state machine via a cached task. AdbcCommand overrides ExecuteDbDataReaderAsync and awaits AdbcStatement.ExecuteQueryAsync, sharing behavior validation with ExecuteReader. Where blocking legitimately remains, on the synchronous APIs, it now uses AsTask(). A driver's stream is genuinely asynchronous, so reading .Result on the ValueTask it returns is unsupported. That applied to AdbcDataReader.Read and to the schema-loading loop in AdbcConnection, which had the same defect independently of this change. All additive. No driver changes and no public contract change. Read(), GetSchema() and ExecuteDbDataReader are unchanged, and a driver that overrides only ExecuteQuery still gets the base Task.Run implementation. AdbcStatement.ExecuteQueryAsync takes no CancellationToken, so the initial query call stays uncancellable as it is today. Per-batch fetches are cancellable. Closes apache#4715
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the fix described in #4715.
AdbcCommandoverrode onlyExecuteDbDataReaderandAdbcDataReaderonlyRead(), so both BCL async entry points fell back to the synchronous bodies.ReadNextRecordBatchAsyncbecomes genuinely async,AdbcDataReadergains aReadAsyncoverride andAdbcCommandanExecuteDbDataReaderAsyncoverride.Three notes on decisions in the diff, rather than as comments in the source.
ReadAsyncreturns a cached task on the intra-batch path instead of beingasync. It runs once per row, not once per batch, so anasyncmethod would allocate a state machine on the 4095 advances out of every 4096 that complete synchronously. Measured at 0 B per row, identical toRead().Blocking that legitimately remains, on the synchronous APIs, now uses
AsTask(). A driver's stream is genuinely asynchronous, so reading.Resulton theValueTaskit returns is unsupported. That applies toAdbcDataReader.Readand to the schema-loading loop inAdbcConnection, which had the same defect independently of this change.ExecuteDbDataReaderAsynconly checks itsCancellationTokenrather than passing it down, becauseAdbcStatement.ExecuteQueryAsynctakes none. The initial query call stays uncancellable exactly as it is today; per-batch fetches become cancellable throughReadAsync.All additive: no driver changes, no public contract change.
Read(),GetSchema()andExecuteDbDataReaderare unchanged, and a driver overriding onlyExecuteQuerystill gets the baseTask.Runimplementation.Tests are in
Client/ClientTests.cs.ReadAsyncDoesNotDeadlockOnASynchronizationContexthangs against currentmainand passes here.Closes #4715