diff --git a/NEWS.md b/NEWS.md index a67b059..6637d5c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,6 +12,7 @@ * Partition locks never make a write fail when the lock itself cannot be taken: any error while locking (for example a stale network file handle or too many open files, not only file systems without lock support) now warns once and the write proceeds, and an invalid `filearray.lock.timeout` option or `FILEARRAY_LOCK_TIMEOUT` value warns once and is ignored instead of stopping every write * Fixed `mapreduce` (and so `sum`, `max`, `min`, `range` and `fwhich`) reading only the first slice of every partition after the first when `partition_size` is greater than 1, which gave wrong results and wrong `fwhich` indices * `fmap2` reports the error raised by the mapping function instead of "Unknown error." +* Errors and interrupts raised in the functions passed to `fmap`, `fmap2` and `mapreduce` now reach the caller unchanged on Linux builds that compile packages with `libc++` while R itself loads `libstdc++` (for example through ICU). Before, they arrived as "c++ exception (unknown reason)" and could corrupt memory and crash the R session # filearray 0.2.3 diff --git a/src/core.cpp b/src/core.cpp index 82b67e3..2955308 100644 --- a/src/core.cpp +++ b/src/core.cpp @@ -272,8 +272,7 @@ SEXP locationList(const SEXP listOrEnv, const NumericVector& dim, const int stri UNPROTECT(n_protected); // sliceIdx, dots stop("Incorrect subscript dimensions, required: 0, ndim."); } - SET_VECTOR_ELT(sliceIdx, idx_size, PROTECT(CAR(dots))); - n_protected++; + SET_VECTOR_ELT(sliceIdx, idx_size, CAR(dots)); } break; } @@ -310,24 +309,24 @@ SEXP locationList(const SEXP listOrEnv, const NumericVector& dim, const int stri // el = PROTECT(Rf_eval( PREXPR(el), PRENV( el ))); // From R 4.5.0, PREXPR and PRENV are deprecated // Directly evaluate the promise instead - el = PROTECT(Rf_eval( el, listOrEnv )); - n_protected++; + // + // The evaluated promise `ev` is PROTECTed only until the + // coerced result is stored into `sliceIdx` (which then + // protects it), so the protection stack stays bounded. + SEXP ev = PROTECT(Rf_eval( el, listOrEnv )); - if( el == R_NilValue ){ - el = PROTECT(Rf_allocVector(REALSXP, 0)); - n_protected++; + if( ev == R_NilValue ){ + el = Rf_allocVector(REALSXP, 0); } else { - el = PROTECT(realToInt64(as(el), 1, dl, strict)); - n_protected++; + el = realToInt64(as(ev), 1, dl, strict); } SET_VECTOR_ELT(sliceIdx, idx_size, el); + UNPROTECT(1); // ev } else { if( el == R_MissingArg || el == R_NilValue ){ - el = PROTECT(seq_len_int64( dl )); - n_protected++; + el = seq_len_int64( dl ); } else { - el = PROTECT(realToInt64(as(el), 1, dl, strict)); - n_protected++; + el = realToInt64(as(el), 1, dl, strict); } SET_VECTOR_ELT(sliceIdx, idx_size, el); } @@ -336,9 +335,8 @@ SEXP locationList(const SEXP listOrEnv, const NumericVector& dim, const int stri sliceIdx = PROTECT(Rf_allocVector(VECSXP, ndims)); n_protected++; for(idx_size = 0; idx_size < ndims; idx_size++ ){ - el = PROTECT(seq_len_int64(dim[idx_size])); - SET_VECTOR_ELT(sliceIdx, idx_size, el); - n_protected++; + // `sliceIdx` protects each element once stored. + SET_VECTOR_ELT(sliceIdx, idx_size, seq_len_int64(dim[idx_size])); } } Rf_setAttrib(sliceIdx, Rf_install("_asis_"), Shield(wrap(true))); @@ -495,7 +493,6 @@ List schedule(const SEXP listOrEnv, // change partition, flush! if( l2 > l1 ){ SEXP tmp = PROTECT(Rf_allocVector(REALSXP, l2 - l1)); - n_protected++; Rf_setAttrib(tmp, R_ClassSymbol, Shield(wrap("integer64"))); lastptr2 = ((int64_t*) REAL(last_dim)) + l1; tmpptr = (int64_t*) REAL(tmp); @@ -511,7 +508,6 @@ List schedule(const SEXP listOrEnv, dim2[ndim2 - 1] = cum_part_sizes[part] - part_start; SEXP idx2tmp = PROTECT(loc2idx(sliceIdx2, dim2)); - n_protected++; idx2s.push_back(idx2tmp); // partitions.insert() @@ -520,6 +516,12 @@ List schedule(const SEXP listOrEnv, idx2len_cum += Rf_xlength(idx2tmp); idx2lens[part_size] = idx2len_cum; part_size++; + // `tmp` is kept alive by the PROTECTed `sliceIdx2` and + // `idx2tmp` by the Rcpp `List idx2s` (whose backing SEXP is + // R-preserved after push_back), so the per-iteration + // protections can be released here to keep the protection + // stack depth bounded. + UNPROTECT(2); // tmp, idx2tmp } part = 0; while(cum_part_sizes[part] < *lastptr || (part > 0 && cum_part_sizes[part-1] >= *lastptr)){ @@ -529,7 +531,6 @@ List schedule(const SEXP listOrEnv, } if( l2 > l1 ){ SEXP tmp = PROTECT(Rf_allocVector(REALSXP, l2 - l1)); - n_protected++; Rf_setAttrib(tmp, R_ClassSymbol, Shield(wrap("integer64"))); lastptr2 = ((int64_t*) REAL(last_dim)) + l1; tmpptr = (int64_t*) REAL(tmp); @@ -545,14 +546,15 @@ List schedule(const SEXP listOrEnv, dim2[ndim2 - 1] = cum_part_sizes[part] - part_start; SEXP idx2tmp = PROTECT(loc2idx(sliceIdx2, dim2)); - n_protected++; idx2s.push_back(idx2tmp); // partitions.push_back(part); partitions[part_size] = part; idx2len_cum += Rf_xlength(idx2tmp); idx2lens[part_size] = idx2len_cum; part_size++; - + // `tmp` kept alive by the PROTECTed `sliceIdx2`, `idx2tmp` by the + // Rcpp `List idx2s` (R-preserved after push_back). + UNPROTECT(2); // tmp, idx2tmp } if( part_size > 0 ){ part_size--; diff --git a/src/load.cpp b/src/load.cpp index 81f8891..35e1f92 100644 --- a/src/load.cpp +++ b/src/load.cpp @@ -243,11 +243,14 @@ SEXP FARR_subset_sequential( } } catch (const Rcpp::LongjumpException& e) { - std::rethrow_exception(std::current_exception()); + // Use `throw;`, never `std::rethrow_exception`: a package + // built with libc++ can run in an R that loaded libstdc++, + // and `exception_ptr` would mix the two C++ runtimes + throw; } catch (const boost::interprocess::interprocess_exception& e) { // unable to find the file, skip } catch (const std::exception& e) { - std::rethrow_exception(std::current_exception()); + throw; } catch (...) { throw std::runtime_error("filearray C++: Caught an unknown exception in `FARR_subset_sequential`."); } diff --git a/src/map.cpp b/src/map.cpp index 3e9b558..81c8515 100644 --- a/src/map.cpp +++ b/src/map.cpp @@ -136,9 +136,12 @@ SEXP FARR_buffer_map( // allocate buffers + // `argbuffers` is PROTECTed once; every element stored via SET_VECTOR_ELT + // is protected by the container, so the elements are not PROTECTed + // individually (this keeps the protection stack depth bounded). SEXP argbuffers = PROTECT(Rf_allocVector(VECSXP, narrays)); for(int ii = 0; ii < narrays; ii++){ - SET_VECTOR_ELT(argbuffers, ii, PROTECT(Rf_allocVector(memory_buffer_types[ii], buffer_nelems[ii]))); + SET_VECTOR_ELT(argbuffers, ii, Rf_allocVector(memory_buffer_types[ii], buffer_nelems[ii])); } int64_t current_pos = 0; @@ -210,17 +213,19 @@ SEXP FARR_buffer_map( } catch (const Rcpp::LongjumpException& e) { // An R error or interrupt in `map` (including a partition lock // time-out): let R continue the jump, which also resets the - // protection stack - std::rethrow_exception(std::current_exception()); + // protection stack. Use `throw;`, never `std::rethrow_exception`: + // a package built with libc++ can run in an R that loaded + // libstdc++, and `exception_ptr` would mix the two C++ runtimes + throw; } catch(std::exception &ex){ - UNPROTECT(2 + narrays); + UNPROTECT(2); forward_exception_to_r(ex); } catch(...){ - // UNPROTECT(2 + narrays); + // UNPROTECT(2); Rcpp::warning("C++ `FARR_buffer_map`: cannot finish map"); } - UNPROTECT(2 + narrays); + UNPROTECT(2); return(R_NilValue); } @@ -282,9 +287,12 @@ SEXP FARR_buffer_map2( // allocate buffers + // `argbuffers` is PROTECTed once; every element stored via SET_VECTOR_ELT + // is protected by the container, so the elements are not PROTECTed + // individually (this keeps the protection stack depth bounded). SEXP argbuffers = PROTECT(Rf_allocVector(VECSXP, narrays)); for(int ii = 0; ii < narrays; ii++){ - SET_VECTOR_ELT(argbuffers, ii, PROTECT(Rf_allocVector(memory_buffer_types[ii], buffer_nelems[ii]))); + SET_VECTOR_ELT(argbuffers, ii, Rf_allocVector(memory_buffer_types[ii], buffer_nelems[ii])); } int64_t current_pos = 0; @@ -320,19 +328,19 @@ SEXP FARR_buffer_map2( } catch (const Rcpp::LongjumpException& e) { // An R error or interrupt in `map`: let R continue the jump with // the original condition, which also resets the protection stack - std::rethrow_exception(std::current_exception()); + throw; } catch(std::exception &ex){ - UNPROTECT(2 + narrays); + UNPROTECT(2); forward_exception_to_r(ex); } catch(...){ - UNPROTECT(2 + narrays); + UNPROTECT(2); stop("Unknown error."); } } - UNPROTECT(2 + narrays); + UNPROTECT(2); return(ret); } diff --git a/src/mapreduce.cpp b/src/mapreduce.cpp index efcc383..11cb28d 100644 --- a/src/mapreduce.cpp +++ b/src/mapreduce.cpp @@ -63,9 +63,12 @@ SEXP each_partition_template( UNPROTECT( 1 ); // item } } catch (const Rcpp::LongjumpException& e) { - std::rethrow_exception(std::current_exception()); + // Use `throw;`, never `std::rethrow_exception`: a package + // built with libc++ can run in an R that loaded libstdc++, + // and `exception_ptr` would mix the two C++ runtimes + throw; } catch (const std::exception& e) { - std::rethrow_exception(std::current_exception()); + throw; } catch (...) { throw std::runtime_error("filearray C++: Caught an unknown exception in `each_partition_template`."); } @@ -208,11 +211,11 @@ SEXP FARR_buffer_mapreduce( } } } catch (const Rcpp::LongjumpException& e) { - std::rethrow_exception(std::current_exception()); + throw; } catch (const boost::interprocess::interprocess_exception& e) { // unable to find the file, skip } catch (const std::exception& e) { - std::rethrow_exception(std::current_exception()); + throw; } catch (...) { throw std::runtime_error("filearray C++: Caught an unknown exception in `FARR_buffer_mapreduce`."); } @@ -232,9 +235,9 @@ SEXP FARR_buffer_mapreduce( return(re); } catch (const Rcpp::LongjumpException& e) { - std::rethrow_exception(std::current_exception()); + throw; } catch (const std::exception& e) { - std::rethrow_exception(std::current_exception()); + throw; } catch (...) { throw std::runtime_error("filearray C++: Caught an unknown exception in `FARR_buffer_mapreduce`."); } diff --git a/tests/testthat/test-map.R b/tests/testthat/test-map.R index 583741b..1a72c17 100644 --- a/tests/testthat/test-map.R +++ b/tests/testthat/test-map.R @@ -293,3 +293,35 @@ test_that("fmap2 passes on errors raised by the mapping function", { sum(input[[1]]) }, .buffer_count = 2), c(21, 57)) }) + +test_that("mapreduce passes on errors raised by map and reduce", { + x <- filearray_create(temp_path(check = TRUE), c(2, 3, 2), partition_size = 1) + on.exit(x$delete(force = TRUE), add = TRUE) + x[] <- 1:12 + + expect_error(mapreduce(x, function(data) { + stop("boom from mapreduce map") + }), "boom from mapreduce map") + + expect_error(mapreduce(x, function(data, size) { + sum(data[seq_len(size)]) + }, reduce = function(mapped) { + stop("boom from mapreduce reduce") + }), "boom from mapreduce reduce") + + # the condition itself reaches the caller, not only its message + cond <- structure( + class = c("mapreduce_test_error", "error", "condition"), + list(message = "custom condition", call = NULL) + ) + expect_error(mapreduce(x, function(data) { + stop(cond) + }), class = "mapreduce_test_error") + + # and mapreduce still works afterwards + expect_equal(mapreduce(x, function(data, size) { + sum(data[seq_len(size)]) + }, reduce = function(mapped) { + sum(unlist(mapped)) + }), 78) +})