RagingKore opened a new issue, #4715:
URL: https://github.com/apache/arrow-adbc/issues/4715
## What happened?
`Apache.Arrow.Adbc.Client` is the ADO.NET wrapper over ADBC. It never calls
the
asynchronous methods that the layers above and below it already provide. An
`await`ing
caller reaches the wire through blocking calls, two of them `.Result`.
Three defects, all in `csharp/src/Client/`, verified against `main` at
`b6fcd135`.
**1. `AdbcDataReader.ReadNextRecordBatchAsync` blocks on an already-async
stream.**
`AdbcDataReader.cs:392` declares a private method returning
`ValueTask<RecordBatch?>`
whose name ends in `Async`. Line 396 is its body:
```csharp
RecordBatch? recordBatch =
this.adbcQueryResult.Stream?.ReadNextRecordBatchAsync(cancellationToken).Result;
```
`QueryResult.Stream` is an `IArrowArrayStream` (`Results.cs:51`). Its
`ReadNextRecordBatchAsync(CancellationToken)` is asynchronous by contract
and takes a
token. The reader blocks on it, and discards the token, once per record
batch.
**2. `AdbcDataReader` does not override `ReadAsync`.**
`AdbcDataReader.cs:325` overrides `Read()` only. `DbDataReader.ReadAsync`
falls back to
the base implementation, which runs the synchronous body and reaches the
`.Result` at
line 339.
**3. `AdbcCommand` does not override `ExecuteDbDataReaderAsync`.**
`AdbcCommand.cs:207` overrides `ExecuteDbDataReader` only.
`ExecuteReaderAsync` falls
back to the synchronous body, which calls `AdbcStatement.ExecuteQuery()`.
`AdbcStatement.ExecuteQueryAsync()` already exists at `AdbcStatement.cs:86`
and is
`virtual`. Nothing in the Client calls it.
A fourth occurrence of the same pattern sits at `AdbcConnection.cs:554`, in
the
schema-loading path. It is a separate code path and I have left it out of
the fix
below, but it is the same defect.
### It deadlocks; it does not only block
`.Result` blocks the calling thread until the task completes. The task
completes on a
continuation. Without `ConfigureAwait(false)` that continuation is posted
back to the
captured `SynchronizationContext` — the thread already blocked in `.Result`.
Neither
side can proceed.
I probed this rather than assuming it. The probe drives the real
`AdbcConnection`,
`AdbcCommand`, and `AdbcDataReader` from the shipped
`Apache.Arrow.Adbc.Client` through
their public API. Only the `IArrowArrayStream` is a fake, and its one
distinguishing
feature is an `await` without `ConfigureAwait(false)`, matching
`FlightSqlResult.cs:54`.
Each scenario has a five-second timeout. Its source is in the reproduction
section.
| | Scenario | Expected | Observed |
|---|---|---|---|
| A | `reader.Read()` on a UI-style `SynchronizationContext` | hang |
**HUNG** |
| B | `await reader.ReadAsync(ct)` on the same | hang | **HUNG** |
| C | `reader.Read()` with no `SynchronizationContext` | complete |
completed |
| D | as A, but the stream uses `ConfigureAwait(false)` | complete |
completed |
C is the control, and shows the probe is not simply broken. D isolates the
cause: A and
D differ only by `ConfigureAwait(false)`, so the deadlock needs both the
context capture
below the wrapper and the `.Result` inside it.
B is the case worth emphasising. The caller wrote `await`.
`DbDataReader.ReadAsync` has
no override, so the base implementation runs `Read()` inline on the calling
thread and
deadlocks anyway. Writing asynchronous code does not avoid this.
Nothing on this path calls `ConfigureAwait(false)`:
| Directory | `await` | `ConfigureAwait(false)` |
|---|---:|---:|
| `csharp/src/Client` | 0 | 0 |
| `csharp/src/Drivers/FlightSql` | 4 | 0 |
| `csharp/src/Apache.Arrow.Adbc` | 6 | 0 |
Other parts of the tree use it heavily — 16 calls in `BigQueryStatement.cs`,
and more
throughout the Databricks CloudFetch, Thrift, and Telemetry code, 70 in
total. The
omission on this path looks inconsistent rather than deliberate.
`Apache.Arrow.Adbc.Client` targets `netstandard2.0`, so it ships to .NET
Framework
hosts. WinForms, WPF, and classic ASP.NET each install a single-threaded
`SynchronizationContext`. Any of them reaches scenario A or B.
On ASP.NET Core and console hosts no `SynchronizationContext` exists, so the
call
returns, as scenario C shows. It still parks a thread-pool thread for every
record
batch, which starves the pool under concurrency.
### Why this is a defect, not a feature request
Item 1 stands on its own terms. A private method named
`ReadNextRecordBatchAsync`,
returning `ValueTask<RecordBatch?>`, taking a `CancellationToken`, whose
body calls
`.Result` on an asynchronous method, is wrong whatever the caller wants. It
blocks a
thread and drops the token.
Taken alone, items 2 and 3 are additive improvements rather than defects.
`DbDataReader`
and `DbCommand` ship working base implementations, and a provider that
declines to
override them breaks no contract.
The three compose into a defect. Items 2 and 3 route an awaiting caller into
item 1, and
the result is the deterministic hang shown above. A fallback path that
deadlocks is not a
missing feature.
### Drivers that are already asynchronous
Two in-tree drivers implement `ExecuteQueryAsync` for real: Flight SQL at
`FlightSqlStatement.cs:37`, and HiveServer2/Databricks at
`HiveServer2Statement.cs:166`.
Flight SQL then inverts itself to satisfy the synchronous abstract member —
`FlightSqlStatement.cs:47-50` is `ExecuteQuery() =>
ExecuteQueryAsync().Result`.
Composed through the wrapper, an `await`ing caller runs: BCL synchronous
fallback,
`ExecuteQuery()`, `.Result`, then the genuinely asynchronous method. A
thread blocks for
a gRPC round trip that was already asynchronous.
Item 1 affects every driver, because `IArrowArrayStream` is an Apache Arrow
contract
that every driver's stream satisfies asynchronously.
### Relationship to #1843 and #1865
This is not #1843. That issue asked for a broader asynchronous surface;
#1865 answered
it with the `AdbcStatement11` family, which is asynchronous-primitive and
carries a
`CancellationToken` throughout. That design is sound and this report does
not question it.
This defect sits below it. `AdbcStatement.ExecuteQueryAsync()` and
`AdbcStatement11.ExecuteQueryAsync(CancellationToken)` both return the same
`QueryResult`, holding the same `IArrowArrayStream`. The `.Result` at
`AdbcDataReader.cs:396` is downstream of that join, so moving a driver to
the 1.1 family
does not remove it. Meanwhile every driver shipping today reaches users
through
`AdbcStatement` and this wrapper.
### Proposed fix
Three additive changes in `csharp/src/Client/`, two files. No public
contract change and
no driver change.
1. Make `ReadNextRecordBatchAsync` genuinely `async` and await the stream.
The method is
private, so this changes no contract. `Read()` keeps its `.Result` on it,
which is the
synchronous path's honest cost.
2. Add an `AdbcDataReader.ReadAsync(CancellationToken)` override that awaits
the batch
fetch. Preserve the intra-batch fast path and the dispose-before-fetch
ordering added
by #4133, so a caller retrying after a mid-stream error sees the
exception again
instead of stale rows.
3. Add an `AdbcCommand.ExecuteDbDataReaderAsync` override that awaits
`AdbcStatement.ExecuteQueryAsync()`, sharing the body of
`ExecuteReader(CommandBehavior)`.
Drivers that override only `ExecuteQuery` keep the existing base behaviour,
`Task.Run(() => ExecuteQuery())`, and are unaffected.
Adding `ConfigureAwait(false)` to the driver and core `await` sites would
also help, and
is a separate change.
### Known limitation, left alone deliberately
`AdbcStatement.ExecuteQueryAsync()` takes no `CancellationToken`, so change
3 can only
observe the token at the command boundary. The initial query call stays
uncancellable,
exactly as today. Per-batch fetches become cancellable, and that is where
the repeated
round trips happen.
Adding a token overload to `AdbcStatement` would be new public API on the
core package
and would re-open the design settled in #1865. This report does not propose
it.
---
## Stack Trace
No exception is thrown. This is the managed stack of the deadlocked thread,
captured
from a live process with `dotnet-stack report -p <pid>` while the probe in
the next
field was hung. The thread shown is the one pumping the message loop, which
is what a
WPF Dispatcher or a WinForms message loop is.
```
Thread (0x145D401B):
[Native Frames]
System.Private.CoreLib!System.Threading.WaitSubsystem+ThreadWaitInfo.Wait(int32,bool,bool,value
class LockHolder&)
System.Private.CoreLib!System.Threading.WaitSubsystem.Wait(class
IWaitableObject,class ThreadWaitInfo,int32,bool)
System.Private.CoreLib!System.Threading.WaitHandle.WaitOneNoCheck(int32,bool,class
System.Object,value class WaitHandleWaitSourceMap)
System.Private.CoreLib!System.Threading.Condition.Wait(int32,class
System.Object)
System.Private.CoreLib!System.Threading.ManualResetEventSlim.Wait(int32,value
class System.Threading.CancellationToken)
System.Private.CoreLib!System.Threading.Tasks.Task.SpinThenBlockingWait(int32,value
class System.Threading.CancellationToken)
System.Private.CoreLib!System.Threading.Tasks.Task.InternalWaitCore(int32,value
class System.Threading.CancellationToken)
System.Private.CoreLib!System.Threading.Tasks.Task.InternalWait(int32,value
class System.Threading.CancellationToken)
System.Private.CoreLib!System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(class
System.Threading.Tasks.Task,value class
System.Threading.Tasks.ConfigureAwaitOptions)
System.Private.CoreLib!System.Threading.Tasks.ValueTask`1[System.__Canon].get_Result()
Apache.Arrow.Adbc.Client!Apache.Arrow.Adbc.Client.AdbcDataReader.ReadNextRecordBatchAsync(value
class System.Threading.CancellationToken)
Apache.Arrow.Adbc.Client!Apache.Arrow.Adbc.Client.AdbcDataReader.Read()
hang!PumpSyncContext.Pump()
System.Private.CoreLib!System.Threading.Thread.StartCallback(class
System.Threading.Thread*)
```
Read it bottom-up. The pumping thread is inside `AdbcDataReader.Read()`,
inside
`AdbcDataReader.ReadNextRecordBatchAsync`, inside
`ValueTask<T>.get_Result()`, blocked in
`Task.InternalWait`. Two frames of `Apache.Arrow.Adbc.Client` sit directly
above
`get_Result`.
The cycle it is stuck in:
```
┌───────────────────────┐ ┌────────────────────────┐
│ │ │ continuation of the │
│ UI thread parked in │ waits for │ await at │
│ .Result at ├───────────────►│ FlightSqlResult.cs:54, │
│ AdbcDataReader.cs:396 │ │ queued on the │
│ │ │ context │
└───────────────────────┘ └────────────┬───────────┘
▲can only run on │
│ │
╰─────────────────────────────────────────╯
```
For reference, the same path traced by line:
```
caller: await command.ExecuteReaderAsync(ct)
-> DbCommand.ExecuteDbDataReaderAsync(...) BCL default -> calls
the SYNC method
-> AdbcCommand.ExecuteDbDataReader(...)
Client/AdbcCommand.cs:207 (only override)
-> AdbcCommand.ExecuteReader(behavior)
Client/AdbcCommand.cs:228
-> AdbcStatement.ExecuteQuery() abstract, synchronous
-> FlightSqlStatement.ExecuteQuery()
Drivers/FlightSql/FlightSqlStatement.cs:47
-> ExecuteQueryAsync().Result
Drivers/FlightSql/FlightSqlStatement.cs:49 BLOCKS
caller: while (await reader.ReadAsync(ct))
-> DbDataReader.ReadAsync(...) BCL default -> calls
the SYNC method
-> AdbcDataReader.Read()
Client/AdbcDataReader.cs:325
-> ReadNextRecordBatchAsync().Result
Client/AdbcDataReader.cs:339 BLOCKS
-> Stream.ReadNextRecordBatchAsync(ct).Result
Client/AdbcDataReader.cs:396 BLOCKS
-> FlightSqlResult.ReadNextRecordBatchAsync(ct)
-> await ...MoveNext(ct)
Drivers/FlightSql/FlightSqlResult.cs:54
no
ConfigureAwait(false) -> continuation posted
back to the blocked
SynchronizationContext
```
---
## How can we reproduce the bug?
What a user writes, in a WPF or WinForms event handler:
```csharp
private async void OnLoadClick(object sender, RoutedEventArgs e)
{
using var command = connection.CreateCommand();
command.CommandText = "SELECT * FROM some_large_table";
using var reader = await command.ExecuteReaderAsync(ct);
while (await reader.ReadAsync(ct)) // never returns
{
...
}
}
```
The UI freezes. Nothing throws and no timeout fires, because the token never
reaches an
overridden `ReadAsync`.
### Runnable probe
Save as `probe.cs` outside the repository, adjust the `#:project` path, then
run with
.NET SDK 10 or later:
```
dotnet run probe.cs
```
Observed on macOS, .NET SDK 10.0.301, `main` at `b6fcd135`:
```
[A] reader.Read() on a UI-style SynchronizationContext
expected: HUNG observed: HUNG OK
[B] await reader.ReadAsync(ct) on a UI-style SynchronizationContext
expected: HUNG observed: HUNG OK
[C] reader.Read() with no SynchronizationContext (console / ASP.NET Core)
expected: completed observed: completed OK
[D] reader.Read() on a UI-style SynchronizationContext, stream uses
ConfigureAwait(false)
expected: completed observed: completed OK
RESULT: all scenarios matched expectations.
```
<details>
<summary>probe.cs</summary>
```csharp
#:property PublishAot=false
#:property Nullable=disable
#:property TreatWarningsAsErrors=false
#:project ../arrow-adbc/csharp/src/Client/Apache.Arrow.Adbc.Client.csproj
// Everything below the stream fake is the real shipped code, reached
through its
// fully public surface. No internals, no reflection, no patched build.
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Apache.Arrow;
using Apache.Arrow.Ipc;
using Apache.Arrow.Types;
using Adbc = Apache.Arrow.Adbc;
using Client = Apache.Arrow.Adbc.Client;
const int TimeoutMs = 5000;
bool useConfigureAwait = false;
StreamMode.UseConfigureAwait = () => useConfigureAwait;
int failures = 0;
failures += Report("A", "reader.Read() on a UI-style SynchronizationContext",
expectHang: true, RunWithSyncContext(reader => reader.Read()));
failures += Report("B", "await reader.ReadAsync(ct) on a UI-style
SynchronizationContext",
expectHang: true,
RunWithSyncContext(reader =>
reader.ReadAsync(CancellationToken.None).GetAwaiter().GetResult()));
failures += Report("C", "reader.Read() with no SynchronizationContext
(console / ASP.NET Core)",
expectHang: false, RunWithoutSyncContext(reader => reader.Read()));
useConfigureAwait = true;
failures += Report("D", "reader.Read() on a UI-style SynchronizationContext,
stream uses ConfigureAwait(false)",
expectHang: false, RunWithSyncContext(reader => reader.Read()));
useConfigureAwait = false;
Console.WriteLine();
Console.WriteLine(failures == 0
? "RESULT: all scenarios matched expectations."
: $"RESULT: {failures} scenario(s) did NOT match expectations.");
return failures == 0 ? 0 : 1;
static int Report(string id, string what, bool expectHang, bool hung)
{
string got = hung ? "HUNG" : "completed";
string want = expectHang ? "HUNG" : "completed";
Console.WriteLine($"[{id}] {what}");
Console.WriteLine($" expected: {want,-9} observed: {got,-9} {(hung
== expectHang ? "OK" : "MISMATCH")}");
return hung == expectHang ? 0 : 1;
}
// Runs body(reader) on a thread that installs a single-threaded
SynchronizationContext
// and pumps it, as a WPF Dispatcher or a WinForms message loop does.
static bool RunWithSyncContext(Func<Client.AdbcDataReader, bool> body)
{
var finished = new ManualResetEventSlim(false);
var t = new Thread(() =>
{
var ctx = new PumpSyncContext();
SynchronizationContext.SetSynchronizationContext(ctx);
ctx.Post(_ =>
{
try { body(NewReader()); } catch { }
finished.Set();
ctx.Complete();
}, null);
ctx.Pump();
});
t.IsBackground = true;
t.Start();
return !finished.Wait(TimeoutMs);
}
static bool RunWithoutSyncContext(Func<Client.AdbcDataReader, bool> body)
{
var finished = new ManualResetEventSlim(false);
var t = new Thread(() =>
{
SynchronizationContext.SetSynchronizationContext(null);
try { body(NewReader()); } catch { }
finished.Set();
});
t.IsBackground = true;
t.Start();
return !finished.Wait(TimeoutMs);
}
static Client.AdbcDataReader NewReader()
{
var connection = new Client.AdbcConnection(
new ProbeDriver(),
new Dictionary<string, string> { { "probe", "1" } },
new Dictionary<string, string>());
connection.Open();
var command = connection.CreateCommand();
command.CommandText = "SELECT 1";
return command.ExecuteReader();
}
sealed class PumpSyncContext : SynchronizationContext
{
readonly BlockingCollection<KeyValuePair<SendOrPostCallback, object>>
queue = new();
public override void Post(SendOrPostCallback d, object state)
{
try { queue.Add(new KeyValuePair<SendOrPostCallback, object>(d,
state)); }
catch (InvalidOperationException) { }
}
public override void Send(SendOrPostCallback d, object state) =>
d(state);
public void Pump()
{
foreach (var item in queue.GetConsumingEnumerable())
item.Key(item.Value);
}
public void Complete() => queue.CompleteAdding();
}
// The only fake. Its one distinguishing feature is the await below:
// no ConfigureAwait(false), matching
Drivers/FlightSql/FlightSqlResult.cs:54.
sealed class AsyncCapturingStream : IArrowArrayStream
{
readonly Schema schema;
readonly RecordBatch[] batches;
int index = -1;
public AsyncCapturingStream()
{
schema = new Schema(new List<Field> { new Field("n",
Int32Type.Default, true) }, null);
var builder = new Int32Array.Builder();
builder.AppendRange(new List<int> { 1, 2, 3 });
Int32Array array = builder.Build();
batches = new[] { new RecordBatch(schema, new List<IArrowArray> {
array }, array.Length) };
}
public Schema Schema => schema;
public async ValueTask<RecordBatch>
ReadNextRecordBatchAsync(CancellationToken cancellationToken = default)
{
if (StreamMode.UseConfigureAwait())
await Task.Delay(25, cancellationToken).ConfigureAwait(false);
else
await Task.Delay(25, cancellationToken);
index++;
return index < batches.Length ? batches[index] : null;
}
public void Dispose() { }
}
static class StreamMode
{
public static Func<bool> UseConfigureAwait = () => false;
}
sealed class ProbeStatement : Adbc.AdbcStatement
{
public override Adbc.QueryResult ExecuteQuery() => new
Adbc.QueryResult(3, new AsyncCapturingStream());
public override Adbc.UpdateResult ExecuteUpdate() => throw new
NotImplementedException();
}
sealed class ProbeConnection : Adbc.AdbcConnection
{
public override Adbc.AdbcStatement CreateStatement() => new
ProbeStatement();
public override IArrowArrayStream GetObjects(GetObjectsDepth depth,
string catalogPattern,
string dbSchemaPattern, string tableNamePattern,
IReadOnlyList<string> tableTypes,
string columnNamePattern) => throw new NotImplementedException();
public override Schema GetTableSchema(string catalog, string dbSchema,
string tableName) =>
throw new NotImplementedException();
public override IArrowArrayStream GetTableTypes() => throw new
NotImplementedException();
}
sealed class ProbeDatabase : Adbc.AdbcDatabase
{
public override Adbc.AdbcConnection Connect(IReadOnlyDictionary<string,
string> options) => new ProbeConnection();
}
sealed class ProbeDriver : Adbc.AdbcDriver
{
public override Adbc.AdbcDatabase Open(IReadOnlyDictionary<string,
string> parameters) => new ProbeDatabase();
}
```
</details>
---
## Environment/Setup
- Repository: `apache/arrow-adbc`, `main` at `b6fcd135`
- Package: `Apache.Arrow.Adbc.Client`, version prefix `0.25.0-SNAPSHOT`
(`csharp/Directory.Build.props:32`)
- Target frameworks: `netstandard2.0;net8.0`
- Driver used to trace the chain: `Apache.Arrow.Adbc.Drivers.FlightSql`
- OS: macOS. The defect is a property of the source and is platform
independent, but the
deadlock requires a host that installs a `SynchronizationContext`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
