shikokuchuo / mirai

mirai - Minimalist Async Evaluation Framework for R
https://shikokuchuo.net/mirai/
GNU General Public License v3.0
193 stars 10 forks source link

Do not unload namespaces in package cleanup #167

Closed shikokuchuo closed 2 weeks ago

shikokuchuo commented 2 weeks ago

Fixes #166.

@wlandau this is my proposed fix. Please let me know if you can think of any corner cases - I can't think how this change would be problematic. Thanks!

codecov-commenter commented 2 weeks ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.71%. Comparing base (92afc17) to head (d44736d).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #167 +/- ## ======================================= Coverage 99.71% 99.71% ======================================= Files 9 9 Lines 692 692 ======================================= Hits 690 690 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

shikokuchuo commented 2 weeks ago

Note that this would then be consistent with namespaces that are not attached in the first place:

library(mirai)
daemons(1)
#> [1] 1
mirai(loadedNamespaces())[]
#>  [1] "compiler"  "graphics"  "utils"     "grDevices" "stats"     "datasets" 
#>  [7] "methods"   "mirai"     "base"      "nanonext"
mirai(data.table::data.table())[]
#> data frame with 0 columns and 0 rows
mirai(loadedNamespaces())[]
#>  [1] "compiler"   "graphics"   "utils"      "grDevices"  "stats"     
#>  [6] "datasets"   "data.table" "methods"    "mirai"      "base"      
#> [11] "nanonext"
daemons(0)
#> [1] 0

Created on 2024-11-11 with reprex v2.1.1

wlandau commented 2 weeks ago

If this solves #166, then it seems reasonable.

I'm not sure which conditions can reproduce the original data.table-related issue in #166, replicate(100, {library(data.table); detach(name = "package:data.table", unload = TRUE, character.only = TRUE)}) seems to work fine on my end.

shikokuchuo commented 2 weeks ago

The following reliably crashes for me after a few iterations:

for (i in 1:100) {
  library(data.table); data.table(); detach(name = "package:data.table", unload = TRUE, character.only = TRUE)
}