thomasp85 / fiery

A flexible and lightweight web server
https://fiery.data-imaginist.com
Other
240 stars 12 forks source link

Windows CRAN check and win-builder #3

Closed HenrikBengtsson closed 7 years ago

HenrikBengtsson commented 8 years ago

I noticed that fiery 0.2.1 gives an error on the CRAN Windows server, cf.

https://cran.r-project.org/web/checks/check_results_fiery.html

FYI, Uwe Ligges who runs the Windows CRAN checks well as http://win-builder.r-project.org/, (re-)confirmed in person last week that the Windows CRAN machines / tests are identical to the win-builder ones. In other words, you should be able to use win-builder to reproduce the CRAN errors.

thomasp85 commented 8 years ago

Will look into it. Thought AppVeyor would make it safe

thomasp85 commented 7 years ago

Strange as it is this is a problem when running the tests in the testthat environment on a windows builder (works fine locally on windows)...

I've tried poking at testthat but I'm not very knowledgable about how it sets up its environments and runs the tests... Maybe @hadley can chip in though he probably have better things to do :-)

HenrikBengtsson commented 7 years ago

Not 100% sure I'm following what you're checked / saying here. Did you try putting the exact same test code that fails in a tests/issue3.R without the use of testthat? If that passes, i.e. it manages to open a socket, then it's testthat related. I'm not saying it's testthat, but I would like to rule out that part since I think I've seen errors elsewhere that show up under testthat(*) but not when put in plain R tests + the future package runs extensive tests without issues and it does not use testthat.

(*) ... or maybe my memory is mixing it up and that occurred when the tests where running via covr.

thomasp85 commented 7 years ago

It is exactly as you described. Same code but in a source file in the tests directory and everything works on the windows builders. I've also tried putting failing code outside of test_that blocks but inside the testthat files but it still fails

HenrikBengtsson commented 7 years ago

So, you're getting closer and my memory wasn't wrong. I think testthat tries to emulate the test setup, but there are corner cases where it fails. /cc @hadley, does this familiar? (I think covr is doing something similar, maybe even based on testthat code, and I ran into a small number of cases where some calls erred incorrectly under covr::package_coverage() and I had to run those unit tests conditionally.)

As a next step trying to narrow down the issues, you could see if you can reproduce the issue using a plain parallel call, e.g.

cl <- parallel::makePSOCKcluster(2L)
print(cl)
parallel::stopCluster(cl)

It will probably fail for the same reasons.

thomasp85 commented 7 years ago

Putting this in a testthat test results in the check being aborted (just hangs), so it is def something further down from future

HenrikBengtsson commented 7 years ago

Minor comment (and not explaining the issue per se): The reason the pure parallel code "hangs" whereas the fiery w/ future checks "times out" is probably because my future code uses something like:

  connectTimeout <- 2*60
  setTimeLimit(elapsed = connectTimeout)
  on.exit(setTimeLimit(elapsed = Inf))
  socketConnection("localhost", port = port, server = TRUE, blocking = TRUE, open = "a+b", timeout = timeout)

whereas the parallel::makePSOCKcluster() code only uses:

  socketConnection("localhost", port = port, server = TRUE, blocking = TRUE, open = "a+b", timeout = timeout)

PS. Due to a bug in R setTimeLimit() is only effective on Windows, but that is irrelevant since this is a Windows-only issue.

thomasp85 commented 7 years ago

This is what I expected so thanks for confirming that. Apparently it is thus a bug in the intersection between testthat, parallel, and the windows builders...