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

Unhardcode timeout in callRemote(), callFun(), showDialog(). Fixes #291. #301

Closed
wants to merge 1 commit into from

Conversation

jonovik
Copy link

@jonovik jonovik commented Jul 4, 2024

I've added a timeout argument to showDialog() and callRemote() as described below, and added logic to callFun() to propagate a named timeout argument to callRemote() if running as a job.

devtools::check() reports no errors, warnings or notes.

@kevinushey, please have a look. I also added argument descriptions for the roxygen2 comment to callRemote() to please devtools::check(). That documentation is written with the help of ChatGTP, but agrees with my limited understanding of the serialization and sys.call() wizardry.


Revised function signatures:
showDialog(title, message, url = "", timeout = 10) callFun(fname, ...)
callRemote(call, frame, timeout = 10)

Original function signatures:
showDialog(title, message, url = "")
callFun(fname, ...)
callRemote(call, frame)

To avoid adding arguments to callFun(), I make an object of list(...) and check whether it has a timeout element, then callRemote() with two or three arguments as needed.

…tudio#291.

I've added a `timeout` argument to showDialog() and callRemote() as described below, and added logic to callFun() to propagate a named `timeout` argument to callRemote() if running as a job.

Revised function signatures:
showDialog(title, message, url = "", timeout = 10)
callFun(fname, ...)
callRemote(call, frame, timeout = 10)

Original function signatures:
showDialog(title, message, url = "")
callFun(fname, ...)
callRemote(call, frame)

To avoid adding arguments to callFun(), I make an object of list(...) and check whether it has a `timeout` element, then callRemote() with two or three arguments as needed.
@jonovik
Copy link
Author

jonovik commented Jul 5, 2024

My use case is a custom knitr output format which automates interaction with a website using library(selenider).
However, the user needs to log in first, and I use rstudioapi::showDialog() to inform the user about this.
Then I have selenider open the login page. The user logs in, then clicks OK in the dialog I brought up.

Because knitting runs as a "job", I experience the timeout problem described in this issue.
Even if the user confirms that they have successfully logged in, showDialog() will error if more than timeout seconds have passed, and ten seconds is insufficient in this case.
With this pull request, I can set the timeout to a more suitable 120 seconds.


As a simplified test case, create a file called test_show_dialog_timeout.R with the following code:

rstudioapi::showDialog("Test", "Click OK after 15 seconds.")

If you source("test_show_dialog_timeout.R") and click OK after 15 seconds, the code runs without error.
However, the same code will error if run as a background job:
Go to RStudio's "Background Jobs" tab, click "Start Background Job", select test_show_dialog_timeout.R, and wait 15 seconds to click OK.
The output is:

Error in callRemote(sys.call(), parent.frame(), timeout = args$timeout) : 
  RStudio did not respond to rstudioapi IPC request
Calls: .rs.sourceWithProgress ... eval -> eval -> <Anonymous> -> callFun -> callRemote
Execution halted

Then try rerunning the job (green circle arrow in the top right of the Background Jobs pane) and click OK within 5 seconds. This will succeed.


With the code in this pull request, one can extend the timeout and have more time to respond to the dialog:

rstudioapi::showDialog("Test", "Click OK after 15 seconds.", timeout = 30)

It's worth noting that in all cases the dialog will sit there forever, not interrupting after the timeout has passed.
The only effect of the timeout seems to be marking the result as invalid because it took too long.

@kevinushey
Copy link
Contributor

Thanks for the PR! Some thoughts on potential alternatives:

  • What do you think about supporting an option rstudioapi.remote.timeout option? We could keep the default of 10, but allow that to be set (or overridden) by the user if necessary or appropriate.
  • We could then add an argument to rstudioapi::showDialog(timeout = ), which would then set (and restore) the rstudioapi.remote.timeout option for the duration of the call.

This would also allow us to avoid the need to pull the "timeout" argument out of args() when invoking callRemote(), which feels a bit awkward to me.

@kevinushey
Copy link
Contributor

Superceded by #303; nonetheless I appreciate your taking the time to put this together!

@kevinushey kevinushey closed this Jul 11, 2024
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.

2 participants