[GitHub] [arrow] nealrichardson commented on a change in pull request #8246: ARROW-10071: [R] segfault with ArrowObject from previous session, or saved

[GitBox <gi...@apache.org>]

nealrichardson commented on a change in pull request #8246:
URL: https://github.com/apache/arrow/pull/8246#discussion_r493663218



##########
File path: r/src/arrow_cpp11.h
##########
@@ -157,8 +157,15 @@ struct ns {
 
 template <typename Pointer>
 Pointer r6_to_pointer(SEXP self) {
-  return reinterpret_cast<Pointer>(
-      R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+  void* p = R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp));
+  if (p == nullptr) {

Review comment:
       If we're checking for nullptr here, can we remove the `shared_ptr_is_null` check inside the `shared_ptr()` constructor? Then we can just `$new()` to create the R6 objects.

##########
File path: r/tests/testthat/test-arrow.R
##########
@@ -47,3 +47,12 @@ r_only({
     )
   })
 })
+
+test_that("arrow gracefully fails to load objects from other sessions (ARROW-10071)", {
+  a <- Array$create(1:10)
+  tf <- tempfile(); on.exit(unlink(tf))
+  saveRDS(a, tf)
+
+  b <- readRDS(tf)

Review comment:
       I don't understand why this fails--`a` still exists so I'd expect that a pointer to it would still work, even if it had been written to a file--but I trust you that it does :)

##########
File path: r/src/arrow_cpp11.h
##########
@@ -157,8 +157,15 @@ struct ns {
 
 template <typename Pointer>
 Pointer r6_to_pointer(SEXP self) {
-  return reinterpret_cast<Pointer>(
-      R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp)));
+  void* p = R_ExternalPtrAddr(Rf_findVarInFrame(self, arrow::r::symbols::xp));
+  if (p == nullptr) {
+    SEXP klass = Rf_getAttrib(self, R_ClassSymbol);
+    std::string first_class(Rf_isNull(klass) ? "ArrowObject"
+                                             : CHAR(STRING_ELT(klass, 0)));

Review comment:
       1) if this is an R6 object, how can class ever be NULL?
   
   2) Why not take the last element in `klass`, i.e. the actual subclass?




----------------------------------------------------------------
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.

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