rustyconover commented on issue #397:
URL: https://github.com/apache/arrow-dotnet/issues/397#issuecomment-5738548624
`ArrowStreamWriter` hits this hazard internally. A caller that writes a
batch it built only to write gets a `NullReferenceException`, or silently wrong
bytes, with no span in its own code. The writer case also shows that preferring
`Memory<byte>` over `Span` isn't enough on `main`.
### Where it happens
In `WriteRecordBatchInternal`, the last use of `recordBatch` is
`recordBatch.Length`, when the header is built. The bodies are copied later, by
`WriteBufferData(recordBatchBuilder.Buffers)`. In optimized code, if the caller
holds no other reference to the batch, it is unreachable during that copy.
`recordBatchBuilder.Buffers` holds `ReadOnlyMemory<byte>` views. They root
the `NativeMemoryManager`, but not the `SharedMemoryHandle` that owns it. A
collection in that window finalizes the handles, `SharedMemoryHandle.Release()`
disposes the manager, and `WriteBufferData` copies from freed memory. Keeping
`Memory<byte>` alive does not help, because the finalizer that frees the memory
belongs to the handle, not to the manager.
### Repro (Apache.Arrow 23.0.0 from NuGet, .NET 10, linux-arm64)
```csharp
// A destination whose writes allocate, e.g. a network or compression stream.
sealed class CollectingStream : MemoryStream
{
static void Collect() { GC.Collect(); GC.WaitForPendingFinalizers();
GC.Collect(); }
public override void Write(byte[] b, int o, int c) { Collect();
base.Write(b, o, c); }
public override void Write(ReadOnlySpan<byte> b) { Collect();
base.Write(b); }
}
[MethodImpl(MethodImplOptions.NoInlining)]
static RecordBatch Build() // Int64 + String columns, 1000 rows, from the
builders
{ ... }
[MethodImpl(MethodImplOptions.NoInlining)]
static byte[] WriteTemporary()
{
using var stream = new CollectingStream();
using (var writer = new ArrowStreamWriter(stream, schema, leaveOpen:
true))
{
writer.WriteRecordBatch(Build()); // referenced by nothing but the
call
writer.WriteEnd();
}
return stream.ToArray();
}
```
Twenty iterations with `DOTNET_TieredCompilation=0`, each result read back
and checked value by value:
```
temporary batch (no keep-alive): NullReferenceException: 20
batch kept alive by the caller: ok: 20
```
```
System.NullReferenceException: Object reference not set to an instance of an
object.
at System.SpanHelpers.Memmove(Byte& dest, Byte& src, UIntPtr len)
at System.IO.Stream.Write(ReadOnlySpan`1 buffer)
at CollectingStream.Write(ReadOnlySpan`1 buffer)
at Apache.Arrow.Ipc.ArrowStreamWriter.WriteBufferData(IReadOnlyList`1
buffers)
at
Apache.Arrow.Ipc.ArrowStreamWriter.WriteRecordBatchInternal(RecordBatch
recordBatch)
```
With default tiering it passes, because tier-0 keeps locals alive to the end
of the method.
We found this in a multithreaded HTTP server. Each response writes a few
hundred single-row batches built only to be written, and about 0.7% of our
integration test files failed intermittently. Code shaped like
`writer.WriteRecordBatch(BuildRow(...))` is common and looks correct.
### What fixed it for us
`GC.KeepAlive(recordBatch)` at the end of `WriteRecordBatchInternal` and
`WriteRecordBatchInternalAsync`, after the bodies are copied. We added the same
guard for `dictionary` in `WriteDictionary` / `WriteDictionaryAsync`. The
dictionary memo already holds it, so that one is defensive.
This is the second case in the repro, moved into the writer. It is carried
as a patch in our fork. With the patched writer, repeated runs of the server
scenario went from 23 crashes in 144 to 0 in 144. I'm happy to open a PR with
that change and a regression test that forces a collection from the destination
stream, if that's useful while the broader ownership question in this issue is
worked out.
Main is unchanged as of 75718c9: no `GC.KeepAlive` in
`ArrowStreamWriter.cs`, and `SharedMemoryHandle` still has its finalizer.
--
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]
