Repository navigation
run the module body only once per dll - #1528
kevinushey wants to merge 4 commits into
Conversation
|
I have a new 'PLAN.md' from the LLM, derived on 'master plus #1526 rebased'. Plan: Implement PR #1528 -- run the module body only once per dllSummary of findings
Local verification of the codebase
AssessmentCorrect and well-scoped. The fix addresses the reported duplication at the Concerns
Implementation steps1.
|
0354a9b to
0f6dad8
Compare
|
Here is an updated review of the rebased PR. Plan (v2): Review PR #1528 against the merged PR #1526Summary of findings
Local verification of the codebase
AssessmentPR #1528 is in order and remains valid on top of the updated #1526. The
The #1528 test is the current, more robust form: it locates the DLL that owns Concerns
Implementation stepsBecause #1528 is already present and correct, this plan requires no code 1. Confirm the snapshot contents (done)
2. Optional, only if explicitly instructed
Verification
Open questions
|
|
I made the suggested one-line change to ChangeLog, and also remove two redundant 'date' paragraph headers. |
|
I think this one is ready but before I approve @kevinushey do you want to eyeball it too just because of the '#1528 (this) stacked on #1526 and #1526 changed lightly' aspect? |
|
I'll give it a gander, thanks! |
A body that registers some classes or functions and then throws left them in the static module, and the initialized flag stayed false, so a later successful load appended a second copy of each. Module::clear() drops the registrations in the catch block so the body always runs against an empty module. The C callables registered by Add() cannot be removed, but running the body again just registers them again.
kevinushey
left a comment
There was a problem hiding this comment.
Re-read this against the merged #1526. The rebase is clean: only the _init() call, the ModuleScope guard and the flag assignment sit inside the if, so the save/restore scope semantics from #1526 are unchanged on the first boot and later boots touch no scope state at all. The XPtr return and BEGIN_RCPP / END_RCPP are as before. The flag lives in the same TU as the static Rcpp::Module, so an unloaded and reloaded DLL starts fresh.
The one open point from the description and from Dirk's write-up was the failure path: a body that registered some members and then threw left them in the static module with the flag still false, so a later successful boot appended them a second time (class_<T>::get_instance() reuses the class already in the module and AddConstructor() / method() push onto it). Rather than leave that on record, 78f5ac3 closes it: the boot function now catches, calls a new Module::clear() that deletes the registered classes and functions, and rethrows, so the body always runs against an empty module. The C callables registered by Add() cannot be undone, but re-running the body registers them again. inst/tinytest/cpp/ModuleInitRetry.cpp throws on its first boot only; without clear() the new assertions report two constructors and two overloads, with it one of each, and the retried module is usable.
The ChangeLog entry sits in the 2026-10-29 block above the #1525 entry, which matches the ordering of the rest of that block.
|
The ❌ in CI was likely spurious: U of I still blackholes IP address at GitHub Actions which had portscanned. I am in discussion with them; their need is understandable but the block is frustrating to the next user getting that IP. In some other use cases I added logic to detect that and switch to the alternate r2u machine but the |
Fixes #1527. Originally stacked on #1526; rebased onto
masteronce that merged.Module("name", PACKAGE = ...)calls the module's boot function every time it is given a module name, and the boot function re-ran theRCPP_MODULEbody each time. The body adds to astatic Rcpp::Modulethat lives as long as the DLL, so every load appended another copy of each constructor and method overload, which shows up inshow()and leaks. See #1527 for the reprex.Change (
inst/include/Rcpp/Module.h,RCPP_MODULE):static boolflag next to the staticRcpp::Modulerecords that the body has completed; later calls of the boot function skip the body and just return the module pointer. The flag lives in the same DLL instance as the module object, so a DLL that is unloaded and loaded again starts over with a fresh module and flag, as it should.Module::clear()(inst/include/Rcpp/module/Module.h) to drop the classes and functions the body registered before throwing, so a later load runs the body against an empty module instead of appending to a half-registered one. The C callables thatModule::Add()registers cannot be removed, but running the body again simply registers them again.As with #1526, this is confined to the headers and applies once a package is rebuilt.
Tests (
inst/tinytest/test_module.R):sourceCpp()has loadeddemoModule, load it twice more withModule(..., mustStart = TRUE)and check thatModuleWorldstill has one constructor and onegreet()overload. It sits at the end of the file becauseModule()redefines the module's reference classes.inst/tinytest/cpp/ModuleInitRetry.cppis a module whose body registers a class and a function and then throws on its first boot only. The test checks thatsourceCpp()fails with that error, then loads the module again and finds one constructor, one method overload, and a working function and object.Verification:
1 constructor(s), 1 overload(s) of inc()on every iteration with this branch;show()lists a single constructor and method.Expected '1', got '3'); withoutModule::clear()the retry tests fail (Expected '1', got '2'). With both,test_module.R(78/78),test_modref.R,test_expose_class.Randtest_module_client_package.Rpass withRunAllRcppTests=yes RunVerboseRcppTests=yes(R 4.6.1, macOS).Checklist
R CMD checkstill passes all tests (module test files run locally as above)