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

Make sure connection is not upgraded too early #162

Merged
merged 1 commit into from
Jul 3, 2018
Merged

Conversation

wch
Copy link
Collaborator

@wch wch commented Jul 3, 2018

This fixes #161. This makes sure that a HTTP connection is not upgraded too early, before all the request headers have been processed.

In the old http-parser code, parser->upgrade was set in the s_headers_almost_done stage (https://github.com/rstudio/httpuv/blob/v1.3.6.2/src/http-parser/http_parser.c#L1567-L1568). This happens when all the request headers have been processed.

Also, in the current version of http-parser, parser->upgrade is set in s_headers_almost_done (https://github.com/rstudio/httpuv/blob/c3978bad/src/http-parser/http_parser.c#L1822-L1825) or in s_on_headers_completed (https://github.com/rstudio/httpuv/blob/c3978bad/src/http-parser/http_parser.c#L1884).

In #153, we added isUpgrade(). The problem is that it could return true before all the headers have been processed. This PR makes sure that isUpgrade() will return true only after all the headers have been processed.

One thing that I'm not 100% sure about: In the http-parser code, parser->upgrade could be set in a slightly earlier stage (s_headers_almost_done) than what I've done here (HttpRequest:: _on_headers_complete), and I wonder if that could result in different behavior.

Testing notes

  1. Open a new project on https://rstudio.cloud
  2. Install Shiny from CRAN (install.packages("shiny")). The httpuv version from CRAN should be 1.4.4.2; if that's not what packageVersion("httpuv") gives you, then install.packages("httpuv") as well.
  3. shiny::runExample("01_hello")

You should see it grey out. If not, try reloading the app (not the IDE) a few times.

Then install httpuv from GitHub (devtools::install_github("rstudio/httpuv")) and restart R, and try to repro again. It should no longer grey out.

@jcheng5 jcheng5 merged commit ad3b6ff into master Jul 3, 2018
@wch wch deleted the fix-upgrade branch July 4, 2018 03:56
@vsrdharca
Copy link

vsrdharca commented Oct 5, 2021

@wch Hello Winston -

This is an old issue but even though I have most recent shiny and httpuv packages I am encountering a websocket error.
running:
shiny - 1.7.1
httpuv - 1.6.3.9000
rstudio cloud - RStudio Server Version 1.3.959

I run shiny::runExample("01_hello") and get the grey screen with the websocket connection error as in the attached image. on an older version of rstudio server 1.2.5042 I don't have get this error. I would appreciate any thoughts you might have about resolving this.

Much thanks!

image

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.

Shiny apps sometimes crash on rstudio.cloud with httpuv 1.4.4.2
3 participants