Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

devtools::test crashes RStudio when called twice on package using testthat #1832

Closed
KallyopeBio opened this issue Jul 25, 2018 · 17 comments
Closed
Labels

Comments

@KallyopeBio
Copy link

@KallyopeBio KallyopeBio commented Jul 25, 2018

RStudio (as well as R sessions run from the terminal) crashes when devtools::test is invoked twice in succession on a package that uses Catch, via package testthat. The crash occurs in testthat/include/testthat/vendor/catch.h.

I've attached a trivial package that reproduces the problem:
ttcrash.tar.gz

Exact steps to reproduce:
1.) Simply install the package to "some_directory".
2.) > "library(devtools)"
3.) > test("some_directory/ttcrash")
4.) > test("some_directory/ttcrash")

The crash will occur on the last step. I'm using R version 3.4.4 and testthat version 2.0.0

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

I've posted the same issue on the github repo for testthat

@jimhester
Copy link
Member

@jimhester jimhester commented Jul 25, 2018

I cannot reproduce this, the attached package tests without on both Windows and MacOS, with both the CRAN release of devtools as well as the devel version.

Could you provide your sessioninfo::session_info() when you observe the crashing?

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

OK. I will do so now. Please note that I observe this crash on Linux (specifically, Ubuntu 16.04)

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

Here is the session info:
session_info.txt

I invoked the session_info() function after the first call to test(.) but before the second call to test(.), which then caused the R session to crash

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

I can also e-mail you a tar.gz file with the core dump and my R binary if it helps. I cannot attach it here because the file size exceeds 10 MB

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Jul 25, 2018

This might be just r-lib/pkgload#35

@jimhester
Copy link
Member

@jimhester jimhester commented Jul 25, 2018

I can reproduce it on linux with both CRAN devtools and devel devtools, the relevant backtrace is

#0  0x0000000003f2ab40 in ?? ()
#1  0x00007ffff18510da in Catch::seedRng (config=...) at /usr/local/lib/R/site-library/testthat/include/testthat/vendor/catch.h:8652
#2  0x00007ffff1859c15 in run (this=0x7ffff1a91920 <testthat::catchSession()::instance>) at /usr/local/lib/R/site-library/testthat/include/testthat/vendor/catch.h:7115
#3  run_tests () at /usr/local/lib/R/site-library/testthat/include/testthat/testthat.h:118
#4  run_testthat_tests () at /usr/local/lib/R/site-library/testthat/include/testthat/testthat.h:151
#5  0x00007ffff7884bdd in ?? () from /usr/lib/R/lib/libR.so

I am not that familiar with this code, but it seems Catch is using a singleton and it is crashing when trying to initialize the RNG the second time. Perhaps the first singleton is not being destructed properly. @kevinushey any ideas?

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

I confirm that's precisely the backtrace from the crash as it occurs on my machine, as well.

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Jul 25, 2018

I'll take a look!

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Jul 25, 2018

I suspect that part of the issue is that dyn.unload() does not actually unload the associated DLL on Linux (for libraries containing static variables within inline functions):

> library(testthat)
> system(paste("lsof -p", Sys.getpid(), "| grep testthat.so"))
R       9125 kevin  mem    REG    8,1  4112576 1709656 /home/kevin/R/x86_64-pc-linux-gnu-library/3.4/testthat/libs/testthat.so
> dyn.unload(system.file("libs/testthat.so", package = "testthat"))
> system(paste("lsof -p", Sys.getpid(), "| grep testthat.so"))
R       9125 kevin  mem    REG    8,1  4112576 1709656 /home/kevin/R/x86_64-pc-linux-gnu-library/3.4/testthat/libs/testthat.so

Some discussion of this issue on the RedHat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1215452

The GCC manpage has a flag -fno-gnu-unique that helps control behavior around how static data objects are initialized on unload / reload:

-fno-gnu-unique
On systems with recent GNU assembler and C library, the C++ compiler uses the STB_GNU_UNIQUE binding to make sure that definitions of template static data members and static local variables in inline functions are unique even in the presence of RTLD_LOCAL; this is necessary to avoid problems with a library used by two different RTLD_LOCAL plugins depending on a definition in one of them and therefore disagreeing with the other one about the binding of the symbol. But this causes dlclose to be ignored for affected DSOs; if your program relies on reinitialization of a DSO via dlclose and dlopen, you can use -fno-gnu-unique.

But asking package authors to make this change isn't quite realistic. I'll have to think of a better solution.

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

I'm open to incorporating a workaround into my package. If I set that compile flag in the makefile for my package, will that resolve the problem?

@KallyopeBio
Copy link
Author

@KallyopeBio KallyopeBio commented Jul 25, 2018

I just tested the above workaround: I included the -fno-gnu-unique flag during the compilation of my package. It does indeed fix the crash. I can run test(.) an arbitrary number of times in a row, with no problem.

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Jul 25, 2018

I think my PR in r-lib/testthat#779 will fix this without requiring extra compiler flags.

@jimhester
Copy link
Member

@jimhester jimhester commented Jul 25, 2018

Wow that is quite a strange one, I was not aware of this gcc behavior.

If this is a gcc specific issue I wonder why I did not observe this on Windows as well, maybe just deterministic behavior and I did not try re-running test() enough to trigger it.

I wonder if we should have pkgload include -fno-gnu-unique when compiling packages with gcc, or if static variables in inline functions are rare enough case we don't need to worry about it.

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Jul 25, 2018

I think static variables in inline functions should be relatively rare. There's also the worry that (in theory) if a package is tested locally with -fno-gnu-unique but published to CRAN without that flag active, that some usages of that package would fail across unload + load.

I suspect this issue will be rare enough that the issue is worth punting on until someone actually reports similar trouble.

FWIW I went this route (inline function with static variable) just to minimize the amount of boilerplate needed in R packages that wanted to use Catch.

@jimhester
Copy link
Member

@jimhester jimhester commented Jul 25, 2018

Great thanks again Kevin for looking into this, I would not have thought of this being an issue, so I am glad you had an inkling of where to look.

@lock
Copy link

@lock lock bot commented Jan 21, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants