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

Remove reqwest dependency #354

Merged
merged 15 commits into from
Sep 23, 2024
Merged

Remove reqwest dependency #354

merged 15 commits into from
Sep 23, 2024

Conversation

KendallWeihe
Copy link
Contributor

No description provided.

@KendallWeihe
Copy link
Contributor Author

Converting to a draft for the time being for further consideration

@KendallWeihe
Copy link
Contributor Author

I think we'll probably move forward with this, but @diehuxx wondering if you can give it a look. Windows test seems to be failing so I'm guessing something in our TLS or TCP (or something else?) is causing issues specifically on Windows, so I'll get to the bottom of that.

I may rework the http.rs module to be a slightly different design, but I wanted to spike in the ground a functional solution where all tests are passing (except apparently Windows?).

The major win here is to get us off of OpenSSL entirely. Also it reduces the byte code file size by ~10% which is not insignificant. We're unable to get out of ring showing up in our Cargo.lock file, but @finn-tbd just recently added CI jobs which will fail if it slips into the compiled result.

@diehuxx
Copy link
Contributor

diehuxx commented Sep 17, 2024

@KendallWeihe The failure is that we expect Web5Error::Network("failed to lookup address information") but instead we're getting Web5Error::Network("No such host is known"). All that differs is the text of the error message returned to us by TcpStream::Connect in http.rs.

I suspect that Windows rust implementation of TcpStream::connect returns one error message and Mac and Linux return another. In both cases, the error message conveys the same thing, i.e. This host isn't valid.

I'd be comfortable just updating the test to accept either error message. I added a commit doing this.

Ok(json_value)
}

pub fn get_bytes_as_http_response(url: &str) -> Result<HttpResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel like we could just call this get and it would be less confusing. get_bytes_as_http_response sounds like i'm parsing the url as an http response

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! and then do the same for the put_ function below too, yeah?

@KendallWeihe KendallWeihe marked this pull request as ready for review September 18, 2024 12:46
Copy link
Contributor

@nitro-neal nitro-neal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would always be in favor of using a library if possible, but removing openssl / reqwest moves us forward so onwards!

@KendallWeihe
Copy link
Contributor Author

On second thought, I want to spend more time further contemplating the implications here. Moving back to a draft for the time being.

@KendallWeihe KendallWeihe marked this pull request as draft September 18, 2024 20:07
@KendallWeihe
Copy link
Contributor Author

A leading alternative idea I have here: use reqwest inside the web5 crate, but compile it out during binding, and over the FFI expose the ability to inject an http client lib into web5 from the target languages native runtime. So in other words, for cross language bindings, we enable bring-your-own http client lib. This may be the best of both worlds; more overhead, but we compile away openssl/ring/etc so we reduce the byte code file size in the same way, and we're less likely to run into openssl bugs.

@andresuribe87
Copy link
Contributor

A leading alternative idea I have here: use reqwest inside the web5 crate, but compile it out during binding, and over the FFI expose the ability to inject an http client lib into web5 from the target languages native runtime. So in other words, for cross language bindings, we enable bring-your-own http client lib. This may be the best of both worlds; more overhead, but we compile away openssl/ring/etc so we reduce the byte code file size in the same way, and we're less likely to run into openssl bugs.

FWIW, that's similar to the approach that ktor uses to support different platforms. See here, for more details.

@KendallWeihe KendallWeihe marked this pull request as ready for review September 23, 2024 19:31
@KendallWeihe KendallWeihe mentioned this pull request Sep 23, 2024
@diehuxx diehuxx merged commit 1d669a7 into main Sep 23, 2024
20 of 21 checks passed
@diehuxx diehuxx deleted the kendall/rm-reqwest branch September 23, 2024 20:19
diehuxx pushed a commit that referenced this pull request Sep 23, 2024
diehuxx pushed a commit that referenced this pull request Sep 23, 2024
KendallWeihe added a commit that referenced this pull request Sep 24, 2024
* main:
  Revert Revert Remove reqwest dependency (#366)
  Revert "Remove reqwest dependency (#354)" (#365)
  Remove reqwest dependency (#354)
  [TBD Release Manager 🚀] Setting next development version after 0.0.5 to: 0.0.0-main-SNAPSHOT
  [TBD Release Manager 🚀] Setting version to: 0.0.5
  Update release workflow (#362)
  [TBD Release Manager 🚀] Setting next development version after 0.0.1 to: 0.0.0-main-SNAPSHOT
  [TBD Release Manager 🚀] Setting version to: 0.0.1
  remove circular dependency (#360)
  Artifact Publishing (#358)
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.

4 participants