[arrow] branch main updated: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation (#37565)

[pa...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

paleolimbot pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git


The following commit(s) were added to refs/heads/main by this push:
     new 440dc92caa GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation (#37565)
440dc92caa is described below

commit 440dc92caa73ca67c8ca98cebfb74f33788150bf
Author: Dewey Dunnington <de...@voltrondata.com>
AuthorDate: Mon Sep 18 22:50:33 2023 +0200

    GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation (#37565)
    
    ### Rationale for this change
    
    The `gc_memory_pool()` is the one we use almost everywhere in the R package. It uses a special allocation mechanism that calls into R to run the garbage collector after a failed allocation (in case there are any large objects that can be removed). In the case where an allocation happens on another thread (most of the time when running exec plans), the call into R may cause a crash: even though the memory pool was ensuring serialized access using a mutex, this is not sufficient for R ( [...]
    
    ### What changes are included in this PR?
    
    Use `SafeCallIntoR()` to run the garbage collector instead. This ensures that the calling thread is used for any call into R (or errors if this is not possible).
    
    ### Are these changes tested?
    
    Yes: there is an existing test that ensures this code path occurs at least once.
    
    ### Are there any user-facing changes?
    
    No.
    * Closes: #37576
    
    Authored-by: Dewey Dunnington <de...@voltrondata.com>
    Signed-off-by: Dewey Dunnington <de...@fishandwhistle.net>
---
 r/src/memorypool.cpp          | 14 +++++++-------
 r/tests/testthat/test-arrow.R |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/r/src/memorypool.cpp b/r/src/memorypool.cpp
index 027aa8ef2a..696e913ead 100644
--- a/r/src/memorypool.cpp
+++ b/r/src/memorypool.cpp
@@ -16,8 +16,8 @@
 // under the License.
 
 #include <arrow/memory_pool.h>
-#include <arrow/util/mutex.h>
 #include "./arrow_types.h"
+#include "./safe-call-into-r.h"
 
 class GcMemoryPool : public arrow::MemoryPool {
  public:
@@ -59,17 +59,17 @@ class GcMemoryPool : public arrow::MemoryPool {
     if (call().ok()) {
       return arrow::Status::OK();
     } else {
-      auto lock = mutex_.Lock();
-
       // ARROW-10080: Allocation may fail spuriously since the garbage collector is lazy.
       // Force it to run then try again in case any reusable allocations have been freed.
-      static cpp11::function gc = cpp11::package("base")["gc"];
-      gc();
+      arrow::Status r_call = SafeCallIntoRVoid([] {
+        cpp11::function gc = cpp11::package("base")["gc"];
+        gc();
+      });
+      ARROW_RETURN_NOT_OK(r_call);
+      return call();
     }
-    return call();
   }
 
-  arrow::util::Mutex mutex_;
   arrow::MemoryPool* pool_;
 };
 
diff --git a/r/tests/testthat/test-arrow.R b/r/tests/testthat/test-arrow.R
index 071a5ad3d9..c6ae27ac52 100644
--- a/r/tests/testthat/test-arrow.R
+++ b/r/tests/testthat/test-arrow.R
@@ -64,6 +64,6 @@ test_that("MemoryPool calls gc() to free memory when allocation fails (ARROW-100
   on.exit(suppressMessages(untrace(gc)))
   # We expect this should fail because we don't have this much memory,
   # but it should gc() and retry (and fail again)
-  expect_error(BufferOutputStream$create(2**60))
+  expect_error(BufferOutputStream$create(2**60), "Out of memory")
   expect_true(env$gc_was_called)
 })