[arrow-nanoarrow] branch main updated: chore: Suppress more jemalloc thread local storage possible leaks and add memcheck for R to verification (#229)

[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-nanoarrow.git


The following commit(s) were added to refs/heads/main by this push:
     new bc1a133  chore: Suppress more jemalloc thread local storage possible leaks and add memcheck for R to verification (#229)
bc1a133 is described below

commit bc1a133ae8ea89b6a0895a0eca66a58eb1efe1cf
Author: Dewey Dunnington <de...@dunnington.ca>
AuthorDate: Tue Jun 13 12:04:07 2023 -0300

    chore: Suppress more jemalloc thread local storage possible leaks and add memcheck for R to verification (#229)
    
    As reported in #200, when loading the arrow R package and allocating at
    least once there can be an additional thread local storage allocation by
    jemalloc that leaks. I wasn't able to replicate this in Docker
    (ubuntu:22.04 or debian:testing, which I believe is the platform CRAN
    uses); however, their appearance shouldn't cause nanoarrow's memcheck to
    fail (R or otherwise).
    
    In trying to replicate this, I also realized that release verification
    wasn't running the R tests with valgrind. On CI this isn't possible
    because we install arrow from RSPM and that build crashes R under
    valgrind for some reason. The verification job happens on Docker,
    though, so we can add the check there.
    
    Closes #200.
---
 ci/docker/ubuntu.dockerfile             |  2 +-
 dev/release/verify-release-candidate.sh | 11 ++++++++++-
 valgrind.supp                           |  7 +++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/ci/docker/ubuntu.dockerfile b/ci/docker/ubuntu.dockerfile
index 93d2bc1..b6d5246 100644
--- a/ci/docker/ubuntu.dockerfile
+++ b/ci/docker/ubuntu.dockerfile
@@ -39,7 +39,7 @@ RUN pip3 install pydata-sphinx-theme sphinx breathe build Cython numpy pytest py
 # For R. Note that we install arrow here so that the integration tests for R run
 # in at least one test image.
 RUN mkdir ~/.R && echo "MAKEFLAGS += -j$(nproc)" > ~/.R/Makevars
-RUN R -e 'install.packages(c("blob", "hms", "tibble", "rlang", "testthat", "tibble", "vctrs", "withr", "pkgdown", "covr"), repos = "https://cloud.r-project.org")'
+RUN R -e 'install.packages(c("blob", "hms", "tibble", "rlang", "testthat", "tibble", "vctrs", "withr", "pkgdown", "covr", "pkgbuild"), repos = "https://cloud.r-project.org")'
 
 # Required for this to work on MacOS/arm64
 RUN echo "CXX17FLAGS += -fPIC" >> ~/.R/Makevars
diff --git a/dev/release/verify-release-candidate.sh b/dev/release/verify-release-candidate.sh
index 6617061..d513cf0 100755
--- a/dev/release/verify-release-candidate.sh
+++ b/dev/release/verify-release-candidate.sh
@@ -256,7 +256,7 @@ test_r() {
   # (but the arrow integration tests will run if the arrow package is installed anyway).
   # Using a manual approach because installing pak takes a while on some systems and
   # beacuse the package versions don't matter much.
-  "$R_BIN" -e 'for (pkg in c("blob", "hms", "tibble", "rlang", "testthat", "tibble", "vctrs", "withr")) if (!requireNamespace(pkg, quietly = TRUE)) install.packages(pkg, repos = "https://cloud.r-project.org/")'
+  "$R_BIN" -e 'for (pkg in c("blob", "hms", "tibble", "rlang", "testthat", "tibble", "vctrs", "withr", "pkgbuild")) if (!requireNamespace(pkg, quietly = TRUE)) install.packages(pkg, repos = "https://cloud.r-project.org/")'
 
   show_info "Build the R package source tarball"
 
@@ -275,6 +275,15 @@ test_r() {
   # Runs R CMD check on the tarball
   _R_CHECK_FORCE_SUGGESTS_=false "$R_BIN" CMD check "$R_PACKAGE_TARBALL_NAME" --no-manual
 
+  if [ ${TEST_WITH_MEMCHECK} -gt 0 ]; then
+    show_info "Run R tests with valgrind"
+    pushd "$NANOARROW_SOURCE_DIR"
+    "$R_BIN" \
+      -d "valgrind --tool=memcheck --leak-check=full --suppressions=valgrind.supp --error-exitcode=1" \
+      -e "testthat::test_local('r')"
+    popd
+  fi
+
   popd
 }
 
diff --git a/valgrind.supp b/valgrind.supp
index 206ea28..510778b 100644
--- a/valgrind.supp
+++ b/valgrind.supp
@@ -21,3 +21,10 @@
     ...
     fun:_dl_allocate_tls
 }
+
+{
+    <jemalloc>:Thread locals don't appear to be freed
+    Memcheck:Leak
+    ...
+    fun:__tls_get_addr
+}