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

Use private event loop for chrome_print(async=FALSE) #127

Merged
merged 3 commits into from
Oct 17, 2019

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Aug 1, 2019

Note: The diff is pretty minimal if you ignore whitespace.

This PR takes a dependency on the GitHub master version of later. We're hoping to do a release of later soon, so I'm guessing you'll want to wait for that before merging, but I thought I'd create the PR now in case you have questions.

I have not had a chance to fully document the reasons why blocking-while-pumping-events is dangerous unless you use a private event loop, but it looks like @yihui was already aware?

In Chromote we found it necessary to do a lot more work to get user interrupts (i.e. Esc or Ctrl-C) to be respected quickly and to not leak resources. That might not be too big of a concern in chrome_print; the promises::finally() will likely not execute, but in this case the finally doesn't do anything that isn't also being done in on.exit.

I was going to add unit tests, but given the dependency on Chrome/Chromium, I didn't think they would pass on CRAN (or Travis CI for that matter). These are the tests I ran manually:

# sync
{
  later::later(~message("This should print third"))
  pagedown::chrome_print("https://www.google.com") %>%
    { message("This should print first") }
  message("This should print second")
}
# async
library(promises)

{
  later::later(~message("This should print second"))
  pagedown::chrome_print("https://www.google.com", async = TRUE) %...>%
    { message("This should print third") }
  message("This should print first")
}

@jcheng5 jcheng5 requested review from yihui and RLesur August 1, 2019 22:37
@cderv
Copy link
Collaborator

cderv commented Aug 2, 2019

I was going to add unit tests, but given the dependency on Chrome/Chromium, I didn't think they would pass on CRAN (or Travis CI for that matter)

FWIW we have some tests in crrri that are using chrome on Travis so I guess we could add some tests in pagedown about this. Chrome on Travis in not an issue. I let you see what we have done in crrri and tell us if you think it could be useful here. (and maybe in chromote). I can contribute on that if required.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcheng5 If I understand it correctly, the essential change is that you wrapped the code from line 89 in with_temp_loop_maybe() and indented these lines by two spaces. That sounds good to me. Thank you!

@cderv Please feel free to add tests for Travis CI only. I have done such tests in other packages, e.g. https://github.com/yihui/tinytex/blob/master/tests/test-travis.R Thanks!

Merge branch 'master' into private-event-loop

# Conflicts:
#	DESCRIPTION
@RLesur RLesur merged commit 562113f into master Oct 17, 2019
@RLesur RLesur deleted the private-event-loop branch October 17, 2019 20:13
RLesur added a commit that referenced this pull request Dec 16, 2019
…ose() are not run when async=FALSE. I don't understand why.

The same result could have been obtained with `on.exit(close_ws(), add = TRUE, after = FALSE)` but the `after` argument is quite recent (R 3.5.0), I prefer to avoid it for backward compatibility with R < 3.5.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants