ROBUSTNESS: Now the digest DLL is unloaded when the package is unloaded #42
Conversation
|
Why would we want this? As in: "Sure, nice to have" but what actual real-world problem was caused by this? |
|
For the same reason you want to be able to unload/detach packages. See HenrikBengtsson/Wishlist-for-R#29. It hits people on and off. For me it has prevented me from iterating over CRAN packages checking for certain code properties. Imagine a generic R server session; it will eventually run out of DLL slots if loading and unload random packages. I think the question should be: Why not do/enforce this? |
|
I've been using R approximately as long as you have, and I never needed it. I use littler to get fresh sessions on the command-line. So I am inclined to just close this, your well-esteemed wishlist notwithstanding. This is not a bug in digest; this is a desire of yours to get a wholly different R workflow, and I am not the right person to talk to about this. |
|
To me it's no different than unload a package should unload the namespace and for me it's almost up there with memory leaks; you either have to close the R process to fix it or manually identify forgotten DLLs and close them using But I accept your decision. |
|
It is just not a common workflow. AFAICT nobody ever calls these unload functions. I can fold this in but I just don't see the need or priority. |
|
Yes, please ... yes please. |
|
I will, I will, but today is Rcpp day :) |
|
@HenrikBengtsson @jimhester I had to turn your load-unload.R test off. It interacted badly with Jim's covr package (for which I am using current github master which no longer attaches memoise and hence digest by itself). If either of you know how to do coverage test with this load/unload test (which I am still at best lukewarm about, but it seems to matter to you 😁 ) let me know. Else it remains commented out as coverage wins over it. (And just to be plain: the |
|
You know that lukewarm interests many times have turned into passionate love stories, ehe? Are you sure it's related to unregistering the DLL? I don't see an error for the ## Checkout version before DLL unregistering was implemented
git checkout fcbb3315450b> c <- covr::package_coverage(quiet = FALSE)
* installing *source* package 'digest' ...
** libs
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c aes.c -o aes.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c crc32.c -o crc32.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c digest.c -o digest.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c init.c -o init.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c md5.c -o md5.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c pmurhash.c -o pmurhash.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c raes.c -o raes.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c sha1.c -o sha1.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c sha2.c -o sha2.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c sha256.c -o sha256.o
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -O0 -coverage -c xxhash.c -o xxhash.o
gcc -std=gnu99 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -coverage -o digest.so aes.o crc32.o digest.o init.o md5.o pmurhash.o raes.o sha1.o sha2.o sha256.o xxhash.o -L/usr/lib/R/lib -lR
installing to /tmp/Rtmp9fAr0S/R_LIBS5bb8e335c07/digest/libs
** R
** inst
** tests
** preparing package for lazy loading
** help
*** installing help indices
converting help for package 'digest'
AES example
digest example
hmac example
** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (digest)
Running specific tests for package 'digest'
Running 'AESTest.R'
comparing 'AESTest.Rout' to 'AESTest.Rout.save' ...
files differ in number of lines:
7,8d6
< Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
< cyclic namespace dependency detected when loading 'digest', already loading 'memoise', 'covr', 'digest'
Running 'digestTest.R'
comparing 'digestTest.Rout' to 'digestTest.Rout.save' ...
files differ in number of lines:
7,8d6
< Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
< cyclic namespace dependency detected when loading 'digest', already loading 'memoise', 'covr', 'digest'
Running 'hmacTest.R'
comparing 'hmacTest.Rout' to 'hmacTest.Rout.save' ...
files differ in number of lines:
7,8d6
< Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
< cyclic namespace dependency detected when loading 'digest', already loading 'memoise', 'covr', 'digest'
Running 'num2hexTest.R'
Running 'sha1Test.R'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/aes.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'aes.c'
Lines executed:97.18% of 177
Creating 'aes.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/crc32.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'crc32.c'
Lines executed:47.83% of 46
Creating 'crc32.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/digest.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'digest.c'
Lines executed:79.68% of 251
Creating 'digest.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/init.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'init.c'
Lines executed:100.00% of 5
Creating 'init.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/md5.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'md5.c'
Lines executed:99.25% of 133
Creating 'md5.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/pmurhash.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'pmurhash.c'
Lines executed:78.05% of 41
Creating 'pmurhash.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/raes.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'raes.c'
Lines executed:63.33% of 60
Creating 'raes.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/sha1.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'sha1.c'
Lines executed:99.35% of 153
Creating 'sha1.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/sha256.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'sha256.c'
Lines executed:99.33% of 149
Creating 'sha256.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/sha2.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'sha2.c'
Lines executed:33.21% of 280
Creating 'sha2.c.gcov'
'/usr/bin/gcov' '/home/hb/repositories/forks/digest/src/xxhash.gcno' '-o' \
'/home/hb/repositories/forks/digest/src'
File 'xxhash.c'
Lines executed:52.38% of 378
Creating 'xxhash.c.gcov'
> FYI, I've had a few rare cases where if (!is.element("covr", loadedNamespaces())) {
[... to be skipped ...]
}However, I'm not sure if the above "cyclic namespace dependency" errors can be avoided because of this. @jimhester, comments? > sessionInfo()
R version 3.3.2 (2016-10-31)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.1 LTS
locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C
attached base packages:
[1] stats graphics grDevices utils datasets methods base
loaded via a namespace (and not attached):
[1] lazyeval_0.2.0 magrittr_1.5-9001 rex_1.1.1 tools_3.3.2
[5] withr_1.0.2 memoise_1.0.0 digest_0.6.10 covr_2.2.1 |
|
The if (!is.element("covr", loadedNamespaces())) {
[... to be skipped ...]
}is promising. I can guarantee you that in a local R session shine(package_coverage())fails unless I comment-out that file. Hence the ping to you... |
|
What does "fails" mean? I tried with and without tests/load-unload.R and it works the same. The only oddity I see (in both cases) is that covr only reports on library("covr")
shine(package_coverage())covr::shine(covr::package_coverage())c <- covr::package_coverage()
covr::shine(c)c <- covr::package_coverage(quiet = FALSE)
length(c)
## [1] 1280
covr::shine(c)and so on. I've made sure I've tried with all CRAN versions: > sessionInfo()
R version 3.3.2 (2016-10-31)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.1 LTS
locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C
attached base packages:
[1] stats graphics grDevices utils datasets methods base
loaded via a namespace (and not attached):
[1] lazyeval_0.2.0 magrittr_1.5 rex_1.1.1 tools_3.3.2
[5] withr_1.0.2 memoise_1.0.0 digest_0.6.10.2 covr_2.2.1 |
|
Same platform as yours. It just stops. whereas with current master (and hence commented out function) |
|
No mas: !R> cov <- covr::package_coverage()
Error: Failure in `/tmp/RtmphQb0eB/R_LIBS3440566e1f23/digest/digest-tests/load-unload.Rout.fail`
> ## Test that package can be loaded and unloaded cleanly
>
> if (!is.element("covr", loadedNamespaces())) { # nocov start
+ dlls0 <- getLoadedDLLs()
+ stopifnot(!is.element("digest", loadedNamespaces()),
+ !is.element("digest", names(dlls0)))
+
+ loadNamespace("digest")
+ dlls1 <- getLoadedDLLs()
+ stopifnot(length(dlls1) == length(dlls0) + 1,
+ is.element("digest", names(dlls1)))
+
+ unloadNamespace("digest")
+ dlls2 <- getLoadedDLLs()
+ stopifnot(length(dlls2) == length(dlls0),
+ !is.element("digest", names(dlls2)))
+
+ library("digest")
+ dlls3 <- getLoadedDLLs()
+ stopifnot(length(dlls3) == length(dlls0) + 1,
+ is.element("digest", names(dlls3)))
+
+ unloadNamespace("digest")
+ dlls4 <- getLoadedDLLs()
+ stopifnot(length(dlls4) == length(dlls0),
+ !is.elem
R> |
|
Good. I'm a bit surprised I cannot reproduce the error. It does indeed make sense that having covr running before the test script is launched, then digest and hence the digest DLL should be loaded from the beginning resulting in failed |
|
So @jimhester : Is there a global environment variable or something else I could test for to skip the test? The |
|
Doh... I guess I shouldn't do this kind of work when jet lagged. I'll check back in a few days. |
|
The tests are failing in cran covr as well, which is why you don't get coverage from I looked into this a little and it looks like the error is coming from I don't think there is any currently to test if covr is being run, particularly in this case because the failure is causing the load to fail, and covr is not going to be loaded until the package is loaded. I can add an environment variable when covr is running though r-lib/covr#236 will track it. |
|
Ah, yes -- at Travis I am definitely using 'development covr' as I quickly built a .deb package via my PPA based on your master branch. My tests here, or Henrik's, may not have used that. I will leave the status quo of no longer testing this then. We get coverage for the remainder of digest, and that is sort of the more important point here. If and when you have an env var I can condition on that. |
I know you prefer issue discussions first, but I'd say this one is pretty obvious. With digest (<= 0.6.9.3) we get:
This PR makes sure the digest DLL/SO is unloaded together with the namespace.
See also HenrikBengtsson/Wishlist-for-R#29