Skip to content

make HTTP client configurable / libcurl #283

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

Closed
smirnoal opened this issue Feb 1, 2021 · 11 comments
Closed

make HTTP client configurable / libcurl #283

smirnoal opened this issue Feb 1, 2021 · 11 comments
Labels
enhancement New feature or request

Comments

@smirnoal
Copy link

smirnoal commented Feb 1, 2021

libcurl is known as a very fast and reliable library for producing HTTP requests. It is used widely across other Lambda Runtime API implementations.

aws-lambda-rust-runtime is currently using Hyper for this purpose. It would be good to have data comparing hyper performance with libcurl's to understand which backend is more performant for lambda use case.

If such a comparison has already been done, it is good to document the design choice

@coltonweaver coltonweaver added the enhancement New feature or request label Feb 1, 2021
@softprops
Copy link
Contributor

Heads up. curl is actually actively looking to be implemented in terms of hyper. Soon when you use curl youll actually be using hyper!

https://www.infoq.com/news/2020/10/memory-safe-curl-rust/

@smirnoal
Copy link
Author

smirnoal commented Feb 2, 2021

yeah, I've seen that article. It would be interesting to look at the benchmarking data, comparing the performance. Making decisions basing on data is better than basing on rumours :)

As there is a vivid ecosystem of HTTP clients in Rust, it would be a nice feature to have them easily pluggable.

@calavera
Copy link
Contributor

calavera commented Feb 2, 2021

than basing on rumours

it's not really a rumor, you can build curl from source already with hyper as the backend: curl/curl@47f43e0

Other lambda runtimes use libcurl because the performance of the native http libraries are noticeable bad. However, it comes at the cost of portability. The lambda-cpp library that those runtimes use only compiles on Linux, which kinda makes sense because Lambda runs on Linux, but it makes it impossible to have a decent developer workflow on other native systems without a virtualization layer, which makes running tests and local development significantly slower.

It makes sense to measure the performance of the runtime, but you might want to establish a baseline first.

@smirnoal
Copy link
Author

smirnoal commented Feb 2, 2021

I should have used "guesses" instead of "rumours", apologise for that.

This is not to make developers' life harder, but users' lambdas faster.

I'm not sure I understand the portability issue in this context, could you please elaborate? Does it affect libcurl Rust crate?

@calavera
Copy link
Contributor

calavera commented Feb 2, 2021

Does it affect libcurl Rust crate?

that doesn't affect the Rust crate, it only affects how the Python and JavaScript shims work.

The advantage of the Rust runtime has over those other libraries is that you can use features to select the backend that you want, if you want to use something differently than the default. It should be pretty doable to make libcurl an optional feature that replaces the default hyper backend if you prefer to do so.

@davidbarsky
Copy link
Contributor

While I'm personally, weakly not in favor of supporting libcurl in this Lambda runtime, I'd suggest supporting it in a slightly different manner that Cargo's feature flags (as Cargo's feature flags need to be additive). Specifically, I'd factor out the request and response objects into a separate aws-lambda-types crate and create a aws-lambda-hyper crate that depends on the aws-lambda-types crate and implements the core event loop. I'd then create similar crates (like aws-lambda-curl) for each http client or runtime that customers have enough interest in. This would also reduce how much effort is needed for folks to build their own Lambda runtime, if they so chose.

What do you think, @calavera and @smirnoal?

@smirnoal
Copy link
Author

smirnoal commented Feb 3, 2021

I believe it should be implemented in the language idiomatic way, and looks like the "feature" feature is a good fit for it.
I have no preferences regarding factoring out a crate as opposed to a module or other language constructions. I will leave it to more seasoned rustaceans :-)

@davidbarsky
Copy link
Contributor

I believe it should be implemented in the language idiomatic way, and looks like the "feature" feature is a good fit for it.

Cargo's features are deceiving in that way—they have to be additive. To elaborate, unless the features enable separate modules in the lambda crate, an expression like lambda = { version = "0.3", features = ["curl", "hyper"] would result in a build failure. Worse, the person writing this code doesn't need write that expression directly—that property could emerge if lambda = { version = "0.3", features = ["hyper"] and lambda = { version = "0.3", features = ["curl"] exist anywhere in the dependency graph.


I also want to ask a clarifying question as to why this would be a good feature to support. You said:

As there is a vivid ecosystem of HTTP clients in Rust, it would be a nice feature to have them easily pluggable.

What does this feature enable you to do or are you blocked now? Is it to use the Lambda runtime in a non-asynchronous context or use other, non-Tokio runtimes? I'm asking these probing questions to avoid a potential XY problem.

@smirnoal
Copy link
Author

smirnoal commented Feb 4, 2021

there's no hidden ask. The rationale is to provide the best customer experience.
libcurl is fast and reliable and proven itself in many other runtimes.

If we have data, proving that Hyper is not worse (p99), let us document the design choice and close the issue then

@bahildebrand
Copy link
Contributor

@smirnoal were you able to come up with any data comparing libcurl and hyper?

@bahildebrand
Copy link
Contributor

There hasn't been any activity in this issue for some time, so I'm going to close it. I don't believe we have any data showing the performance improvements, and supporting this would be a decent chunk of work. If you want to look into this further and provide data that would help us balance the amount of work with performance improvements please feel free to reopen the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants