diff --git a/ChangeLog b/ChangeLog index 02f61fba4..79737d708 100644 --- a/ChangeLog +++ b/ChangeLog @@ -41,6 +41,20 @@ * inst/tinytest/cpp/Module.cpp: Add a class deriving from another * inst/tinytest/test_module.R: Test the new checks + * inst/include/Rcpp/Module.h (RCPP_MODULE): Run the module body only the + first time the boot function is called, since it adds to the static + module and every Module() call boots the module again, which duplicated + constructors and methods (#1527) + * inst/include/Rcpp/module/Module.h (Module::clear): New, removes the + registered classes and functions + * inst/include/Rcpp/Module.h (RCPP_MODULE): Clear the module when the + body throws, so that a later load runs the body against an empty module + instead of appending to what the failed run registered + * inst/tinytest/cpp/ModuleInitRetry.cpp: New module whose first load + fails after registering a class and a function + * inst/tinytest/test_module.R: Test loading a module repeatedly, and + loading one again after its body threw + * inst/include/Rcpp/Module.h (RCPP_MODULE): Wrap the module boot function in BEGIN_RCPP / END_RCPP so that an exception thrown while the module initializes raises an R error instead of terminating the session diff --git a/inst/include/Rcpp/Module.h b/inst/include/Rcpp/Module.h index cada7d800..2b57cea27 100644 --- a/inst/include/Rcpp/Module.h +++ b/inst/include/Rcpp/Module.h @@ -637,17 +637,29 @@ namespace Rcpp { #define RCPP_MODULE_BOOT(name) _rcpp_module_boot_##name // the boot function is called from R via .Call(), so exceptions thrown by -// the module body must be turned into R errors -#define RCPP_MODULE(name) \ - void _rcpp_module_##name##_init() ; \ - static Rcpp::Module _rcpp_module_##name( # name ) ; \ - extern "C" SEXP _rcpp_module_boot_##name(){ \ - BEGIN_RCPP \ - Rcpp::internal::ModuleScope _rcpp_module_scope( & _rcpp_module_##name ) ; \ - _rcpp_module_##name##_init( ) ; \ - return Rcpp::XPtr( & _rcpp_module_##name , false ) ; \ - END_RCPP \ - } \ +// the module body must be turned into R errors; it runs every time the +// module is loaded, but the body adds to the static module, so it may only +// run once, and a body that throws part-way leaves nothing behind so that +// the next load does not append to what it registered +#define RCPP_MODULE(name) \ + void _rcpp_module_##name##_init() ; \ + static Rcpp::Module _rcpp_module_##name( # name ) ; \ + static bool _rcpp_module_##name##_initialized = false ; \ + extern "C" SEXP _rcpp_module_boot_##name(){ \ + BEGIN_RCPP \ + if( ! _rcpp_module_##name##_initialized ){ \ + Rcpp::internal::ModuleScope _rcpp_module_scope( & _rcpp_module_##name ) ; \ + try { \ + _rcpp_module_##name##_init( ) ; \ + } catch( ... ) { \ + _rcpp_module_##name.clear() ; \ + throw ; \ + } \ + _rcpp_module_##name##_initialized = true ; \ + } \ + return Rcpp::XPtr( & _rcpp_module_##name , false ) ; \ + END_RCPP \ + } \ void _rcpp_module_##name##_init() // silly little dance to suppress a 'defined but not used variable' warning diff --git a/inst/include/Rcpp/module/Module.h b/inst/include/Rcpp/module/Module.h index a8539d7f2..08537b4e7 100644 --- a/inst/include/Rcpp/module/Module.h +++ b/inst/include/Rcpp/module/Module.h @@ -203,6 +203,22 @@ namespace Rcpp { classes.insert( CLASS_PAIR( name_ , cptr ) ) ; } + // drops everything the module body registered; used when the body + // throws, so that running it again starts from an empty module (the + // C callables registered by Add() cannot be removed, but running the + // body again simply registers them again) + inline void clear(){ + for( MAP::iterator it = functions.begin(); it != functions.end(); ++it ){ + delete it->second ; + } + functions.clear() ; + + for( CLASS_MAP::iterator it = classes.begin(); it != classes.end(); ++it ){ + delete it->second ; + } + classes.clear() ; + } + inline bool has_function( const std::string& m){ // #nocov start return functions.find(m) != functions.end() ; } diff --git a/inst/tinytest/cpp/ModuleInitRetry.cpp b/inst/tinytest/cpp/ModuleInitRetry.cpp new file mode 100644 index 000000000..6829027ba --- /dev/null +++ b/inst/tinytest/cpp/ModuleInitRetry.cpp @@ -0,0 +1,50 @@ +// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*- +// +// ModuleInitRetry.cpp: Rcpp R/C++ interface class library -- module whose +// initialization fails the first time only +// +// Copyright (C) 2026 Kevin Ushey +// +// This file is part of Rcpp. +// +// Rcpp is free software: you can redistribute it and/or modify it +// under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 2 of the License, or +// (at your option) any later version. +// +// Rcpp is distributed in the hope that it will be useful, but +// WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Rcpp. If not, see . + +#include + +using namespace Rcpp; + +class ModuleInitRetry { +public: + ModuleInitRetry() {} + int value() { return 42; } +}; + +int retry_add_one(int x) { + return x + 1; +} + +static int boot_count = 0; + +// registers a class and a function, then throws on the first boot only, so +// the second boot must not find what the first one registered +RCPP_MODULE(moduleInitRetry) { + class_("ModuleInitRetry") + .constructor() + .method("value", &ModuleInitRetry::value) + ; + function("retry_add_one", &retry_add_one); + + if (++boot_count == 1) + throw std::runtime_error("first boot fails"); +} diff --git a/inst/tinytest/test_module.R b/inst/tinytest/test_module.R index 4186cf1e1..165d43884 100644 --- a/inst/tinytest/test_module.R +++ b/inst/tinytest/test_module.R @@ -167,6 +167,18 @@ expect_error( r$get(10L), "not initialized" ) expect_error( Rcpp::sourceCpp("cpp/ModuleInitError.cpp"), "no such class" ) expect_true( Module_scope_is_cleared() ) +## a body that throws after registering some of its classes and functions +## leaves nothing behind, so loading the module again does not end up with +## two copies of what the failed run registered +expect_error( Rcpp::sourceCpp("cpp/ModuleInitRetry.cpp"), "first boot fails" ) +is_retry_module <- function(dll) is.loaded("_rcpp_module_boot_moduleInitRetry", PACKAGE = dll[["name"]]) +dll <- Filter(is_retry_module, getLoadedDLLs())[[1L]] +mod <- Module( "moduleInitRetry", PACKAGE = dll, mustStart = TRUE ) +expect_equal( length( mod$ModuleInitRetry@constructors ), 1L ) +expect_equal( length( mod$ModuleInitRetry@methods$value@nargs ), 1L ) +expect_equal( mod$retry_add_one( 41L ), 42L ) +expect_equal( new( mod$ModuleInitRetry )$value(), 42L ) + # test.Module.flexible.semantics <- function( ){ expect_equal( test_reference( seq(0,10) ), 11L ) expect_equal( test_const_reference( seq(0,10) ), 11L ) @@ -189,3 +201,13 @@ expect_identical( res, c(1, 2, 3) ) ## rather than an error (which used to surface at garbage collection) r <- new( ModuleRandomizer ) expect_identical( r$finalize(), NULL ) + +## loading a module again must not re-run its body, which used to add +## every constructor and method to the static module once more (#1527); +## this redefines the module's classes, so it comes last +is_demo_module <- function(dll) is.loaded("_rcpp_module_boot_demoModule", PACKAGE = dll[["name"]]) +dll <- Filter(is_demo_module, getLoadedDLLs())[[1L]] +mod <- Module( "demoModule", PACKAGE = dll, mustStart = TRUE ) +mod <- Module( "demoModule", PACKAGE = dll, mustStart = TRUE ) +expect_equal( length( mod$ModuleWorld@constructors ), 1L ) +expect_equal( length( mod$ModuleWorld@methods$greet@nargs ), 1L )