Copilot commented on code in PR #291:
URL: https://github.com/apache/arrow-dotnet/pull/291#discussion_r2971723103
##########
src/Apache.Arrow/ArrowBuffer.cs:
##########
@@ -83,22 +82,12 @@ internal bool TryExport(ExportedAllocationOwner newOwner,
out IntPtr ptr)
return true;
}
- if (_memoryOwner is IOwnableAllocation ownable &&
ownable.TryAcquire(out ptr, out int offset, out int length))
- {
- newOwner.Acquire(ptr, offset, length);
- ptr += offset;
- return true;
- }
-
- if (_memoryOwner == null &&
CArrowArrayExporter.EnableManagedMemoryExport)
- {
- var handle = _memory.Pin();
- ptr = newOwner.Reference(handle);
- return true;
- }
-
- ptr = IntPtr.Zero;
- return false;
+ // Pin the memory and let the ExportedAllocationOwner track the
handle.
+ // The caller is responsible for keeping the underlying ArrayData
alive
+ // (via AddReference) so the memory owner is not disposed while
pinned.
+ var handle = Memory.Pin();
+ ptr = newOwner.Reference(handle);
+ return true;
Review Comment:
`TryExport` now unconditionally pins and always returns `true`, which makes
the boolean return value (and callers' `if (!TryExport(...))` checks)
effectively dead code. Also, `Memory.Pin()` can throw (e.g., for non-pinnable
`ReadOnlyMemory`), which previously would have been represented as a `false`
return and a consistent `NotSupportedException` from the exporter. Either
restore a real success/failure contract (catch pinning exceptions and return
`false`), or change `TryExport`/callers to not use a boolean result.
```suggestion
try
{
// Pin the memory and let the ExportedAllocationOwner track
the handle.
// The caller is responsible for keeping the underlying
ArrayData alive
// (via AddReference) so the memory owner is not disposed
while pinned.
var handle = Memory.Pin();
ptr = newOwner.Reference(handle);
return true;
}
catch
{
ptr = IntPtr.Zero;
return false;
}
```
##########
src/Apache.Arrow/Arrays/ArrayData.cs:
##########
@@ -97,8 +101,52 @@ public ArrayData(
Dictionary = dictionary;
}
+ private ArrayData(
+ ArrayData parent,
+ IArrowType dataType,
+ int length, int nullCount, int offset,
+ ArrowBuffer[] buffers, ArrayData[] children, ArrayData dictionary)
+ {
+ _parent = parent;
+ DataType = dataType ?? NullType.Default;
+ Length = length;
+ NullCount = nullCount;
+ Offset = offset;
+ Buffers = buffers;
+ Children = children;
+ Dictionary = dictionary;
+ }
+
+ /// <summary>
+ /// Increment the reference count, allowing this ArrayData to be shared
+ /// across multiple owners. Each call to Acquire must be balanced by a
+ /// call to <see cref="Dispose"/>.
+ /// </summary>
+ /// <returns>This instance.</returns>
+ public ArrayData Acquire()
+ {
+ if (Interlocked.Increment(ref _referenceCount) <= 1)
+ {
+ Interlocked.Decrement(ref _referenceCount);
+ throw new ObjectDisposedException(nameof(ArrayData));
+ }
+ return this;
+ }
+
public void Dispose()
{
+ if (Interlocked.Decrement(ref _referenceCount) != 0)
+ {
+ return;
+ }
Review Comment:
`ArrayData.Dispose()` will silently drive `_referenceCount` negative if
`Dispose()` is called more times than it was acquired (or after it reached 0).
This can hide incorrect lifetime management and make diagnosing ref-counting
bugs harder. Consider detecting `Interlocked.Decrement` results < 0 and
throwing (or at least `Debug.Assert`) to catch double-dispose/mismatched
Acquire/Dispose early.
##########
src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -27,9 +27,10 @@ namespace Apache.Arrow.C
public static class CArrowArrayExporter
{
/// <summary>
- /// Experimental feature to enable exporting managed memory to
CArrowArray. Use with caution.
+ /// Formerly-experimental feature to enable exporting managed memory
to CArrowArray. Now obsolete.
/// </summary>
- public static bool EnableManagedMemoryExport = false;
+ [Obsolete]
Review Comment:
`EnableManagedMemoryExport` is now marked `[Obsolete]` but with no guidance
for callers, and the field appears to be a no-op after the export logic
changes. Consider adding an `Obsolete` message indicating it is
ignored/always-on (and whether/when it will be removed) to reduce confusion for
downstream consumers.
```suggestion
[Obsolete("EnableManagedMemoryExport is obsolete and ignored;
managed memory export is now always enabled and this field will be removed in a
future release.")]
```
##########
src/Apache.Arrow/Arrays/ArrayData.cs:
##########
@@ -149,6 +204,44 @@ public ArrayData Slice(int offset, int length)
return new ArrayData(DataType, length, nullCount, offset, Buffers,
Children, Dictionary);
}
Review Comment:
`Slice(int offset, int length)` constructs a new `ArrayData` that shares
`Buffers/Children/Dictionary` but has `_parent == null`, so both the original
and the slice will each dispose the same underlying `ArrowBuffer` owners when
they are disposed. This is particularly problematic because the public
`Array.Slice` / `ArrowArrayFactory.Slice` / `RecordBatch.Slice` paths use
`ArrayData.Slice`, meaning common slicing patterns can lead to
double-dispose/double-free (and for `NativeMemoryManager`, potentially an
exception when freeing pinned memory during C export). Consider making the
default slice path ref-counted (e.g., implement `Slice` in terms of
`SliceShared`, or update the array/batch slicing helpers to call `SliceShared`)
so slices keep the root buffers alive and disposal is safe.
--
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]
