You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "CurtHagenlocher (via GitHub)" <gi...@apache.org> on 2023/06/08 12:51:41 UTC

[GitHub] [arrow] CurtHagenlocher opened a new pull request, #35996: GH-35988: [C#] The C data interface implementation can leak on import

CurtHagenlocher opened a new pull request, #35996:
URL: https://github.com/apache/arrow/pull/35996

   ### What changes are included in this PR?
   
   Adds a flag to indicate whether or not to free the C structure when API structs are released.
   Relaxes the requirement when exporting that the target structure appear uninitialized.
   
   ### Are these changes tested?
   
   Existing tests pass. We don't as yet seem to have a good way to test for memory leaks so no new tests have been added.
   
   ### Are there any user-facing changes?
   
   A new required parameter was added to several functions. As this code has not yet been part of a release, this isn't a breaking change.
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] eerhardt commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223326388


##########
csharp/src/Apache.Arrow/C/CArrowArrayStreamImporter.cs:
##########
@@ -99,24 +93,17 @@ public ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancell
                 }
 
                 RecordBatch result = null;
-                CArrowArray* cArray = CArrowArray.Create();
-                try
+                CArrowArray cArray = new CArrowArray();
+                fixed (CArrowArrayStream* cArrayStream = &_cArrayStream)
                 {
-                    int errno = _cArrayStream->get_next(_cArrayStream, cArray);
+                    int errno = cArrayStream->get_next(cArrayStream, &cArray);
                     if (errno != 0)
                     {
                         throw new Exception($"Unexpected error recieved from external stream. Errno: {errno}");
                     }
-                    if (cArray->release != null)
+                    if (cArray.release != null)
                     {
-                        result = CArrowArrayImporter.ImportRecordBatch(cArray, _schema);
-                    }
-                }
-                finally
-                {
-                    if (result == null)
-                    {
-                        CArrowArray.Free(cArray);

Review Comment:
   Who is freeing the memory that cArray references now? Or does the `RecordBatch` that gets returned take ownership of this memory now?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on PR #35996:
URL: https://github.com/apache/arrow/pull/35996#issuecomment-1582976839

   > The second change is IMHO the symptom of an implementation issue. `ImportedArrowArray` currently stores the given raw pointer `CArrowArray* _cArray`. Instead, it should probably store its own `CArrowArray` struct like the C++ importer does:
   
   Ah, that's much nicer.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on PR #35996:
URL: https://github.com/apache/arrow/pull/35996#issuecomment-1589695207

   > If I understand correctly the basic premise is that, on import, we let `CArrowArray` be in the garbage collector pool since it should be safe to destroy if we happen to lose all references to it (unlike an exported array which we assume will have external references)? That seems valid to me.
   
   By embedding in the `ImportedArrowArray`, it's allocated as part of that object and will have a lifetime equal to it. This is exactly what we want, because it's the `ImportedArrowArray` which already manages the lifetime of the imported memory.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223333700


##########
csharp/src/Apache.Arrow/C/CArrowArrayStreamImporter.cs:
##########
@@ -99,24 +93,17 @@ public ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancell
                 }
 
                 RecordBatch result = null;
-                CArrowArray* cArray = CArrowArray.Create();
-                try
+                CArrowArray cArray = new CArrowArray();
+                fixed (CArrowArrayStream* cArrayStream = &_cArrayStream)
                 {
-                    int errno = _cArrayStream->get_next(_cArrayStream, cArray);
+                    int errno = cArrayStream->get_next(cArrayStream, &cArray);
                     if (errno != 0)
                     {
                         throw new Exception($"Unexpected error recieved from external stream. Errno: {errno}");
                     }
-                    if (cArray->release != null)
+                    if (cArray.release != null)
                     {
-                        result = CArrowArrayImporter.ImportRecordBatch(cArray, _schema);
-                    }
-                }
-                finally
-                {
-                    if (result == null)
-                    {
-                        CArrowArray.Free(cArray);

Review Comment:
   ImportRecordBatch always took ownership of the array data before. What's changed is that the memory for cArray is now allocated on the stack instead of the heap.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] eerhardt commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223154468


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -42,12 +43,12 @@ public static class CArrowArrayImporter
         /// IArrowArray importedArray = CArrowArrayImporter.ImportArray(importedPtr);
         /// </code>
         /// </examples>
-        public static unsafe IArrowArray ImportArray(CArrowArray* ptr, IArrowType type)
+        public static unsafe IArrowArray ImportArray(CArrowArray* ptr, IArrowType type, bool freeOnRelease)

Review Comment:
   Should `freeOnRelease` have a default value? Or is it imperative that the caller sets it?



##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -29,7 +29,8 @@ public static class CArrowArrayImporter
         /// </summary>
         /// <remarks>
         /// This will call the release callback once all of the buffers in the returned
-        /// IArrowArray are disposed.
+        /// IArrowArray are disposed. If freeOnRelease is set, it will also free the memory

Review Comment:
   Usually comments about the parameters go in a `/// <param name="name">` comment.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223337644


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -90,42 +96,49 @@ public static unsafe RecordBatch ImportRecordBatch(CArrowArray* ptr, Schema sche
 
         private sealed unsafe class ImportedArrowArray : ImportedAllocationOwner
         {
-            private readonly CArrowArray* _cArray;
+            private readonly CArrowArray _cArray;
 
             public ImportedArrowArray(CArrowArray* cArray)
             {
                 if (cArray == null)
                 {
                     throw new ArgumentNullException(nameof(cArray));
                 }
-                _cArray = cArray;
-                if (_cArray->release == null)
+                if (cArray->release == null)
                 {
                     throw new ArgumentException("Tried to import an array that has already been released.", nameof(cArray));
                 }
+                _cArray = *cArray;
+                cArray->release = null;
             }
 
             protected override void FinalRelease()
             {
-                if (_cArray->release != null)
+                if (_cArray.release != null)
                 {
-                    _cArray->release(_cArray);
+                    fixed (CArrowArray* cArray = &_cArray)
+                    {
+                        cArray->release(cArray);
+                    }

Review Comment:
   Without the "fixed", in theory the GC could move the current object while we're inside the call. This would change the location of the embedded struct.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] eerhardt merged pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt merged PR #35996:
URL: https://github.com/apache/arrow/pull/35996


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #35996:
URL: https://github.com/apache/arrow/pull/35996#issuecomment-1595247446

   Conbench analyzed the 5 benchmark runs on commit `1e2dfceb`.
   
   There was 1 benchmark result indicating a performance regression:
   
   - Commit Run on `ursa-thinkcentre-m75q` at [2023-06-16 07:27:47Z](http://conbench.ursa.dev/compare/runs/d0f39c38132f41159fa379cd0dfdb13b...559d5c1f5b9e4d95993be829ed7f3a9b/)
     - [params=<SMALL_VECTOR(std::shared_ptr<int>)>, source=cpp-micro, suite=arrow-small-vector-benchmark](http://conbench.ursa.dev/compare/benchmarks/0648be7bfcb87332800046cf1b2d00de...0648c0f31a7b73838000c96b60713836)
   
   The [full Conbench report](https://github.com/apache/arrow/runs/14332829987) has more details.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223339284


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -90,42 +96,49 @@ public static unsafe RecordBatch ImportRecordBatch(CArrowArray* ptr, Schema sche
 
         private sealed unsafe class ImportedArrowArray : ImportedAllocationOwner
         {
-            private readonly CArrowArray* _cArray;
+            private readonly CArrowArray _cArray;
 
             public ImportedArrowArray(CArrowArray* cArray)
             {
                 if (cArray == null)
                 {
                     throw new ArgumentNullException(nameof(cArray));
                 }
-                _cArray = cArray;
-                if (_cArray->release == null)
+                if (cArray->release == null)
                 {
                     throw new ArgumentException("Tried to import an array that has already been released.", nameof(cArray));
                 }
+                _cArray = *cArray;
+                cArray->release = null;
             }
 
             protected override void FinalRelease()
             {
-                if (_cArray->release != null)
+                if (_cArray.release != null)
                 {
-                    _cArray->release(_cArray);
+                    fixed (CArrowArray* cArray = &_cArray)
+                    {
+                        cArray->release(cArray);
+                    }

Review Comment:
   Yes, I was wondering about the reason. Thank you!



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] adamreeve commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223797124


##########
csharp/src/Apache.Arrow/C/CArrowSchemaExporter.cs:
##########
@@ -50,10 +50,6 @@ public static unsafe void ExportType(IArrowType datatype, CArrowSchema* schema)
             {
                 throw new ArgumentNullException(nameof(schema));
             }
-            if (schema->release != null)
-            {
-                throw new ArgumentException("Cannot export schema to a struct that is already initialized.");

Review Comment:
   For what it's worth, it looks like this change fixes an issue I ran into using a C# exported stream from an older version of C++ Arrow, where the schema struct used when importing the stream was uninitialized: https://github.com/apache/arrow/blob/apache-arrow-10.0.1/cpp/src/arrow/c/bridge.cc#L1782 (this has since been fixed)



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223337644


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -90,42 +96,49 @@ public static unsafe RecordBatch ImportRecordBatch(CArrowArray* ptr, Schema sche
 
         private sealed unsafe class ImportedArrowArray : ImportedAllocationOwner
         {
-            private readonly CArrowArray* _cArray;
+            private readonly CArrowArray _cArray;
 
             public ImportedArrowArray(CArrowArray* cArray)
             {
                 if (cArray == null)
                 {
                     throw new ArgumentNullException(nameof(cArray));
                 }
-                _cArray = cArray;
-                if (_cArray->release == null)
+                if (cArray->release == null)
                 {
                     throw new ArgumentException("Tried to import an array that has already been released.", nameof(cArray));
                 }
+                _cArray = *cArray;
+                cArray->release = null;
             }
 
             protected override void FinalRelease()
             {
-                if (_cArray->release != null)
+                if (_cArray.release != null)
                 {
-                    _cArray->release(_cArray);
+                    fixed (CArrowArray* cArray = &_cArray)
+                    {
+                        cArray->release(cArray);
+                    }

Review Comment:
   Without the "fixed", in theory the GC could move the current object while we're inside the call. This would change the location of the embedded struct. (Edit: also, the compiler requires it -- but I assumed you were more curious about the underlying reason.)



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35996:
URL: https://github.com/apache/arrow/pull/35996#issuecomment-1582941222

   Hmm, so there are two (unrelated?) changes here.
   
   The first change (removing the `cArray->release != null` checks in export functions) looks fine.
   
   The second change is IMHO the symptom of an implementation issue. `ImportedArrowArray` currently stores the given raw pointer `CArrowArray* _cArray`. Instead, it should probably store its own `CArrowArray` struct like the C++ importer does:
   https://github.com/apache/arrow/blob/e920bed4cc0f7826cf979a36283fceed403d2860/cpp/src/arrow/c/bridge.cc#L1241-L1261
   
   Then, importing an array _moves_ the user-supplied structure to the owned structure, like this:
   https://github.com/apache/arrow/blob/e920bed4cc0f7826cf979a36283fceed403d2860/cpp/src/arrow/c/bridge.cc#L1285-L1287
   
   Therefore, the imported array structure does not depend on the lifetime of the user-supplied pointer.
   
   By the way, moving is cheap so you shouldn't fear any inefficiency here:
   https://github.com/apache/arrow/blob/e920bed4cc0f7826cf979a36283fceed403d2860/cpp/src/arrow/c/helpers.h#L66-L75
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223203673


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -42,12 +43,12 @@ public static class CArrowArrayImporter
         /// IArrowArray importedArray = CArrowArrayImporter.ImportArray(importedPtr);
         /// </code>
         /// </examples>
-        public static unsafe IArrowArray ImportArray(CArrowArray* ptr, IArrowType type)
+        public static unsafe IArrowArray ImportArray(CArrowArray* ptr, IArrowType type, bool freeOnRelease)

Review Comment:
   I was originally going to default to false, but maybe forcing the user to think about it isn't a bad idea?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35996:
URL: https://github.com/apache/arrow/pull/35996#issuecomment-1590718105

   As far as I can understand, the code looks good now (but I'm not a C# developer).


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] CurtHagenlocher commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223335379


##########
csharp/src/Apache.Arrow/C/CArrowArrayStreamImporter.cs:
##########
@@ -99,24 +93,17 @@ public ValueTask<RecordBatch> ReadNextRecordBatchAsync(CancellationToken cancell
                 }
 
                 RecordBatch result = null;
-                CArrowArray* cArray = CArrowArray.Create();
-                try
+                CArrowArray cArray = new CArrowArray();
+                fixed (CArrowArrayStream* cArrayStream = &_cArrayStream)
                 {
-                    int errno = _cArrayStream->get_next(_cArrayStream, cArray);
+                    int errno = cArrayStream->get_next(cArrayStream, &cArray);
                     if (errno != 0)
                     {
                         throw new Exception($"Unexpected error recieved from external stream. Errno: {errno}");
                     }
-                    if (cArray->release != null)
+                    if (cArray.release != null)
                     {
-                        result = CArrowArrayImporter.ImportRecordBatch(cArray, _schema);
-                    }
-                }
-                finally
-                {
-                    if (result == null)
-                    {
-                        CArrowArray.Free(cArray);

Review Comment:
   (Also, the CArrowArray was previously being leaked on success which was one of the reasons to take another look here.)



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223336191


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -90,42 +96,49 @@ public static unsafe RecordBatch ImportRecordBatch(CArrowArray* ptr, Schema sche
 
         private sealed unsafe class ImportedArrowArray : ImportedAllocationOwner
         {
-            private readonly CArrowArray* _cArray;
+            private readonly CArrowArray _cArray;
 
             public ImportedArrowArray(CArrowArray* cArray)
             {
                 if (cArray == null)
                 {
                     throw new ArgumentNullException(nameof(cArray));
                 }
-                _cArray = cArray;
-                if (_cArray->release == null)
+                if (cArray->release == null)
                 {
                     throw new ArgumentException("Tried to import an array that has already been released.", nameof(cArray));
                 }
+                _cArray = *cArray;
+                cArray->release = null;
             }
 
             protected override void FinalRelease()
             {
-                if (_cArray->release != null)
+                if (_cArray.release != null)
                 {
-                    _cArray->release(_cArray);
+                    fixed (CArrowArray* cArray = &_cArray)
+                    {
+                        cArray->release(cArray);
+                    }

Review Comment:
   I'm curious, why not simply:
   ```suggestion
                       _cArray.release(&_cArray);
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] eerhardt commented on a diff in pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #35996:
URL: https://github.com/apache/arrow/pull/35996#discussion_r1223327425


##########
csharp/src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -90,42 +96,49 @@ public static unsafe RecordBatch ImportRecordBatch(CArrowArray* ptr, Schema sche
 
         private sealed unsafe class ImportedArrowArray : ImportedAllocationOwner
         {
-            private readonly CArrowArray* _cArray;
+            private readonly CArrowArray _cArray;
 
             public ImportedArrowArray(CArrowArray* cArray)
             {
                 if (cArray == null)
                 {
                     throw new ArgumentNullException(nameof(cArray));
                 }
-                _cArray = cArray;
-                if (_cArray->release == null)
+                if (cArray->release == null)
                 {
                     throw new ArgumentException("Tried to import an array that has already been released.", nameof(cArray));
                 }
+                _cArray = *cArray;
+                cArray->release = null;
             }
 
             protected override void FinalRelease()
             {
-                if (_cArray->release != null)
+                if (_cArray.release != null)
                 {
-                    _cArray->release(_cArray);
+                    fixed (CArrowArray * cArray =  &_cArray)

Review Comment:
   (nit)
   ```suggestion
                       fixed (CArrowArray* cArray =  &_cArray)
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] github-actions[bot] commented on pull request #35996: GH-35988: [C#] The C data interface implementation can leak on import

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35996:
URL: https://github.com/apache/arrow/pull/35996#issuecomment-1582526745

   :warning: GitHub issue #35988 **has been automatically assigned in GitHub** to PR creator.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org