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

Removes the httpuv connection: close workaround #318

Closed

Conversation

atheriel
Copy link
Contributor

@atheriel atheriel commented Oct 9, 2018

This removes the workaround introduced in 4901c5e to avoid rstudio/httpuv#49, which has been fixed (in my estimation) since at least httpuv 1.4.0.

I've tentatively confirmed that environments are no longer shared between connections even when they are keep-alive by using the original integration test added in the referenced commit.

As a nice bonus, this improves plumber's baseline latency by about 10% on my machine.

This has been fixed since at least httpuv version 1.4.0.
@CLAassistant
Copy link

CLAassistant commented Oct 9, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Oct 9, 2018

Codecov Report

Merging #318 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #318      +/-   ##
========================================
- Coverage   90.01%    90%   -0.01%     
========================================
  Files          25     25              
  Lines        1142   1141       -1     
========================================
- Hits         1028   1027       -1     
  Misses        114    114
Impacted Files Coverage Δ
R/response.R 100% <ø> (ø) ⬆️
R/plumber.R 84.74% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c25b8b...bd44115. Read the comment docs.

@wch
Copy link
Collaborator

wch commented Oct 10, 2018

Looks good to me.

@trestletech
Copy link
Contributor

There's one more file in the commit you referenced: 4901c5e#diff-68678c8c847a2f72255e66a25514f0abR34 should we also update that one?

@schloerke
Copy link
Collaborator

schloerke commented Oct 26, 2018

@atheriel Thank you for the change! Could you please sign the CLA for when I merge the PR next week? Link: https://cla-assistant.io/trestletech/plumber?pullRequest=318

@atheriel
Copy link
Contributor Author

@schloerke Ah, I didn't realize I'd need to sign for this change, since I did not contribute any copyrightable material or code. It should show as signed now.

schloerke added a commit that referenced this pull request Nov 5, 2018
* Removes the httpuv connection: close workaround.

This has been fixed since at least httpuv version 1.4.0.

* update test file that isn't tested
schloerke added a commit to schloerke/plumber that referenced this pull request Nov 6, 2018
* master:
  atheriel - remove connection close workaround (rstudio#318, rstudio#332)
schloerke added a commit to schloerke/plumber that referenced this pull request Nov 6, 2018
* master:
  atheriel - remove connection close workaround (rstudio#318, rstudio#332)
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.

6 participants