rstudio / httpuv

HTTP and WebSocket server package for R
Other
229 stars 86 forks source link

Biocondcutor BrowserViz base class breaks with version >= 1.4.0 #125

Closed paul-shannon closed 5 years ago

paul-shannon commented 6 years ago

Just upgrading now to 1.4.1, and discovered that the ws handshake, the promotion from http, is never established between httpuv in R, and javascript in the browser. This - alas! - breaks my bioc BrowserViz subclass packages: igvR, RCyjs.

Same problem with 1.4.0 Success, however, when backing off to 1.3.6.2. I am running R 3.4.4, MacOS Sierra (10.12.3), Chrome 65.0.3325.181

Before I dig into this (I'm on the hook to get this working before the Bioconductor release next week), can anyone offer me insight, point me in the right direction? Maybe there's a 1.3->1.4 upgrade guide?

Thanks.

wch commented 6 years ago

Sorry, we don't have an upgrade guide. Can you point us to the code where this is happening?

paul-shannon commented 6 years ago

@wch Thanks for your quick response. The current release is here: http://bioconductor.org/packages/3.7/bioc/html/BrowserViz.html It should build from the source tarball provided there, depending only on jsonlite and httpuv. You can reproduce success, then the problem (no handshake, no promotion from http to ws) following these steps:

The websocket-related R side of the process is all in R/BrowserViz-class.R, see

I could reduce this all to a few dozen lines each of R and javascript - just say the word.

wch commented 6 years ago

@paul-shannon It would help if you could make an example where I could cut and paste it to run - thanks.

wch commented 6 years ago

Stepping through the code, I have a suspicion about what's happening.

This is some of the code for BrowserViz():

BrowserViz = function(portRange=10000:10100, title="BrowserViz",  browserFile, quiet=TRUE,
                      httpQueryProcessingFunction=NULL)
{
  host <- "localhost"

  wsCon <- new.env(parent=emptyenv())

  result <- .startDaemonizedServerOnFirstAvailableLocalHostPort(portRange, wsCon)

...

  wsCon <- .setupWebSocketHandlers(wsCon, browserFile, quiet)

...
}

First it creates the server (in .startDaemonizedServerOnFirstAvailableLocalHostPort), passing an empty wsCon environment. And then later on, it defines the callbacks in wsCon (with .setupWebSocketHandlers. When startServer() and startDaemonizedServer() are called, they expect the app object to have all the callbacks defined at that time. I'm actually a little surprised that it worked this way in the past.

If you can call .setupWebSocketHandlers() before passing the wsCon object to startServer, that may fix the issue. It would be safest if wsCon were a list, to avoid the temptation of modifying it after passing it to startServer(). If you absolutely must have the callbacks be mutable after creating the server, you could create an app object where each function simply is a wrapper that calls out to another function that you could modify/replace later on. Something roughly like:

wsCon <- list(
  call = function(req) {
    myCall(req)
  }
)

startServer(...., wsCon)

myCall <- function(req) {
  # Do something here
}
wch commented 6 years ago

Also, here is an email that we had sent out to some users of httpuv about the big changes in this version. This doesn't touch on your specific problem, but it still may be helpful.


The big change to httpuv is that the network I/O is now handled on a background thread, so I/O no longer block when the main R thread is busy. (Note that the main R thread is still needed to execute the application's callbacks for handling requests, so R must be unblocked to handle requests.)

Previously, after calling startServer(), you would need to repeatedly call service() to handle requests. Now, when startServer() is called, httpuv will launch the background thread if needed, and start listening for requests. When it receives a request, it will schedule the R application callbacks to execute on the main R thread. The scheduled callbacks will run if the console is idle (that is, if there's nothing on R's call stack and it is sitting at the prompt), or if httpuv::service() is called. (A bit more detail: service() is now a thin wrapper around later::run_now(), so all you really need to do is call later::run_now() to execute the R application callbacks).

Most existing code that uses httpuv has this form, and will continue to work:

  startServer()
  while(T) {
    service()
  }

However, you can also simply do this, and if R is idle, it will work:

  startServer()

The old startDaemonizedServer() function did something similar to the new startServer(), except that startServer uses a separate thread for I/O. In the current version, startDaemonizedServer is simply an alias for the startServer function.

paul-shannon commented 6 years ago

Thanks, Winston. I will dig into this first thing tomorrow morning, keeping you posted on my progress. Thanks for taking the time to help out.

On Apr 23, 2018, at 3:06 PM, Winston Chang notifications@github.com wrote:

Stepping through the code, I have a suspicion about what's happening.

This is some of the code for BrowserViz():

BrowserViz = function(portRange=10000:10100, title="BrowserViz", browserFile, quiet=TRUE ,

httpQueryProcessingFunction=NULL ) {

host <- "localhost"

wsCon <- new.env(parent= emptyenv())

result <- .startDaemonizedServerOnFirstAvailableLocalHostPort(portRange, wsCon )

...

wsCon <- .setupWebSocketHandlers(wsCon, browserFile, quiet )

...

}

First it creates the server (in .startDaemonizedServerOnFirstAvailableLocalHostPort), passing an empty wsCon environment. And then later on, it defines the callbacks in wsCon (with .setupWebSocketHandlers. When startServer() and startDaemonizedServer() are called, they expect the app object to have all the callbacks defined at that time. I'm actually a little surprised that it worked this way in the past.

If you can call .setupWebSocketHandlers() before passing the wsCon object to startServer, that may fix the issue. It would be safest if wsCon were a list, to avoid the temptation of modifying it after passing it to startServer(). If you absolutely must have the callbacks be mutable after creating the server, you could create an app object where each function simply is a wrapper that calls out to another function that you could modify/replace later on. Something roughly like:

wsCon <- list (

call = function(req ) { myCall( req ) } )

startServer( ...., wsCon )

myCall <- function(req ) {

Do something here

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

paul-shannon commented 6 years ago

Hi Winston,

I tried your first suggestion - to call .setupWebSocketHandlers before startServer. That makes more sense than my earlier approach, but alias it does not solve my problem.

I think that instead the problem lies in assumptions I made which are at odds with the new “I/O on a background thread” design.

Can you point me towards a minimal example that exemplifies the new approach, setting up message passing between R and the browser?

Thank you -

On Apr 23, 2018, at 3:08 PM, Winston Chang notifications@github.com wrote:

Also, here is an email that we had sent out to some users of httpuv about the big changes in this version. This doesn't touch on your specific problem, but it still may be helpful.

The big change to httpuv is that the network I/O is now handled on a background thread, so I/O no longer block when the main R thread is busy. (Note that the main R thread is still needed to execute the application's callbacks for handling requests, so R must be unblocked to handle requests.)

Previously, after calling startServer(), you would need to repeatedly call service() to handle requests. Now, when startServer() is called, httpuv will launch the background thread if needed, and start listening for requests. When it receives a request, it will schedule the R application callbacks to execute on the main R thread. The scheduled callbacks will run if the console is idle (that is, if there's nothing on R's call stack and it is sitting at the prompt), or if httpuv::service() is called. (A bit more detail: service() is now a thin wrapper around later::run_now(), so all you really need to do is call later::run_now() to execute the R application callbacks).

Most existing code that uses httpuv has this form, and will continue to work:

startServer() while(T) { service() }

However, you can also simply do this, and if R is idle, it will work:

startServer()

The old startDaemonizedServer() function did something similar to the new startServer(), except that startServer uses a separate thread for I/O. In the current version, startDaemonizedServer is simply an alias for the startServer function.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

paul-shannon commented 6 years ago

httpuv/demo/daemon-echo.R seems to be the example I need.

(I should have checked on this before writing my last note.)

On Apr 24, 2018, at 10:38 AM, Paul Shannon paul.thurmond.shannon@gmail.com wrote:

Hi Winston,

I tried your first suggestion - to call .setupWebSocketHandlers before startServer. That makes more sense than my earlier approach, but alias it does not solve my problem.

I think that instead the problem lies in assumptions I made which are at odds with the new “I/O on a background thread” design.

Can you point me towards a minimal example that exemplifies the new approach, setting up message passing between R and the browser?

Thank you -

  • Paul

On Apr 23, 2018, at 3:08 PM, Winston Chang notifications@github.com wrote:

Also, here is an email that we had sent out to some users of httpuv about the big changes in this version. This doesn't touch on your specific problem, but it still may be helpful.

The big change to httpuv is that the network I/O is now handled on a background thread, so I/O no longer block when the main R thread is busy. (Note that the main R thread is still needed to execute the application's callbacks for handling requests, so R must be unblocked to handle requests.)

Previously, after calling startServer(), you would need to repeatedly call service() to handle requests. Now, when startServer() is called, httpuv will launch the background thread if needed, and start listening for requests. When it receives a request, it will schedule the R application callbacks to execute on the main R thread. The scheduled callbacks will run if the console is idle (that is, if there's nothing on R's call stack and it is sitting at the prompt), or if httpuv::service() is called. (A bit more detail: service() is now a thin wrapper around later::run_now(), so all you really need to do is call later::run_now() to execute the R application callbacks).

Most existing code that uses httpuv has this form, and will continue to work:

startServer() while(T) { service() }

However, you can also simply do this, and if R is idle, it will work:

startServer()

The old startDaemonizedServer() function did something similar to the new startServer(), except that startServer uses a separate thread for I/O. In the current version, startDaemonizedServer is simply an alias for the startServer function.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

paul-shannon commented 6 years ago

Hi Winston,

I think I am getting closer to seeing the nature of the problem. Maybe you can see a solution?

Putting i/o on a separate thread seems to have broken the trick I used to simulate synchronous function calls from R to the web browser.

Using getBrowserWindowTitle(app) as an example:

 1) in R, BrowserViz::getBrowserWindowTitle sets a global state variable 
    (a bit like your .lastMessage) to NULL

 2) getBrowserWindowTitle then sends a message to javascript; javascript sends a response

 3) in R, the javascript message is routed to a function called “handleResponse”,
    which sets the global state variable to the non-null value returned from javascript

 4) Meanwhile BrowserViz::getBrowserWindowTitle sits in a tight sleep loop (0.1 seconds) until
    the state variable is no longer null

 5) BrowserViz::getBrowserWindowTitle then returns the value of that global state value 
    to the caller, thus appearing to be a synchronous call to the web browser

Due (I think) to the new threaded design, this loop never exits:

while(is.null(globalStateVariable)){ Sys.sleep(0.1) }

Is there some state of the separate i/o thread - or something else - I can check so that

Thank you!

On Apr 23, 2018, at 3:08 PM, Winston Chang notifications@github.com wrote:

Also, here is an email that we had sent out to some users of httpuv about the big changes in this version. This doesn't touch on your specific problem, but it still may be helpful.

The big change to httpuv is that the network I/O is now handled on a background thread, so I/O no longer block when the main R thread is busy. (Note that the main R thread is still needed to execute the application's callbacks for handling requests, so R must be unblocked to handle requests.)

Previously, after calling startServer(), you would need to repeatedly call service() to handle requests. Now, when startServer() is called, httpuv will launch the background thread if needed, and start listening for requests. When it receives a request, it will schedule the R application callbacks to execute on the main R thread. The scheduled callbacks will run if the console is idle (that is, if there's nothing on R's call stack and it is sitting at the prompt), or if httpuv::service() is called. (A bit more detail: service() is now a thin wrapper around later::run_now(), so all you really need to do is call later::run_now() to execute the R application callbacks).

Most existing code that uses httpuv has this form, and will continue to work:

startServer() while(T) { service() }

However, you can also simply do this, and if R is idle, it will work:

startServer()

The old startDaemonizedServer() function did something similar to the new startServer(), except that startServer uses a separate thread for I/O. In the current version, startDaemonizedServer is simply an alias for the startServer function.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

wch commented 6 years ago

@paul-shannon Can you try using service(0.1) instead of Sys.sleep(0.1)?

paul-shannon commented 6 years ago

@wch Thanks, Winston - that gets me a bit further. Other oddities persist, likely my own fault. I'll keep digging.

paul-shannon commented 6 years ago

@wch service() works nicely in place of sleep - thanks. My new problem with 1.4.1: I now see TWO ws connections created: onWSOpen is called twice. This is reported both in R and in the browser (both in Chrome and Firefox). I cannot yet reproduce this in a tiny example - but perhaps you have seen it before? Double connections lead to flakey behavior when messages start passing back and forth.

FWIW - maybe not much - a web search of "websocket connection opens twice" returns some possibly related information. One stackoverflow post suggests that there may be ready state information available in the newly-created connection, and that the connection from the browser is open, but not ready on the first wsONOpen callback.

paul-shannon commented 6 years ago

@wch The double connection was my coding error.

Things are looking up. I hope to close this issue by the end of the day. Thanks again.

wch commented 6 years ago

Great, I'm glad that things are working for you!

paul-shannon commented 6 years ago

@wch My packages are building clean now. Many thanks.

One lingering question: in some of my unit tests there is a > 10 second lag between

paul-shannon commented 5 years ago

My questions were answered and or resolved long ago. Many thanks for the fine package.