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

Add new default 'libcurl' transport #348

Merged
merged 13 commits into from Mar 25, 2019
Merged

Add new default 'libcurl' transport #348

merged 13 commits into from Mar 25, 2019

Conversation

jmcphers
Copy link
Member

@jmcphers jmcphers commented Mar 19, 2019

This change adds a new HTTP transport, "libcurl", and makes it the default. This new transport uses the curl package, which is significantly more modern and better maintained than RCurl. Consequently we're also able to restore a unified HTTP transport default across all operating systems (we formerly had to use the command-line "curl" transport on Windows due to issues with the aging RCurl).

The diff is noisier than it needs to be since it includes some file splitting; http.R had grown to over 1200 lines. Each transport now has its own file.

The other source of noise is a complete re-write of how we handle streaming logs from shinyapps.io, necessitated because curl and RCurl handle streaming differently. Thankfully the new implementation is much more succinct and doesn't require a child R process.

Closes #325.

@jmcphers jmcphers requested a review from scottmmjackson Mar 22, 2019
Copy link
Contributor

@scottmmjackson scottmmjackson left a comment

Verified 👍

@jmcphers jmcphers merged commit fef0bf6 into master Mar 25, 2019
2 checks passed
@jeroen
Copy link
Contributor

@jeroen jeroen commented Mar 25, 2019

Looks great! One note: I see you are manually parsing cookies from the header, are you aware libcurl has a native function for this called handle_cookies()? See https://cran.r-project.org/web/packages/curl/vignettes/intro.html#reading_cookies

@jmcphers
Copy link
Member Author

@jmcphers jmcphers commented Mar 25, 2019

Yes, I saw that! Very useful. However we have our own internal cookie implementation that we use on all the other transports, and re-using it was more straightforward than adding additional state management to keep curl handles around.

@jeroen
Copy link
Contributor

@jeroen jeroen commented Apr 26, 2019

You could also consider dropping RCurl as hard dependency by moving it into Suggests. Because besides not working well, it's also a pain to install on some systems. On Windows it requires a special system setup to install from source.

@jmcphers
Copy link
Member Author

@jmcphers jmcphers commented May 13, 2019

@jeroen That's a good idea; done: 35ba049

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.

3 participants