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

Replace Hyper with Warp for HTTP server #1998

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

aarroyoc
Copy link
Contributor

@aarroyoc aarroyoc commented Sep 5, 2023

Changes:

  • Use Warp instead of Hyper (Warp still uses Hyper underneath, but manages more things for us)
  • Optimize clones
  • HTTPS support, just by passing a key and a certificate
  • Content-Length limit
  • HTTP Basic Auth
  • Stop server with Ctrl-C

@aarroyoc
Copy link
Contributor Author

aarroyoc commented Sep 5, 2023

(in streams.rs) Is it really needed to do a std::ptr::drop_in_place in the case of HttpRead and HttpWrite? NamedTcp and NamedTls don't do it but InputFile and OutputFile do it but I can't find the difference on why some have it and the others don't

@mthom
Copy link
Owner

mthom commented Sep 5, 2023

I'm not sure why either! To be safe and not leak memory or resources, all stream instances should do drop_in_place.

@aarroyoc
Copy link
Contributor Author

aarroyoc commented Sep 5, 2023

I want to take ownership of the response buffer of the headers on drop to send them to Warp without a clone. And that's when I realized that we're only dropping after that the response buffer but not the headers. Also, now that I transfer ownership to Warp I'm not sure if we still need to do the drop ourselves...

@mthom
Copy link
Owner

mthom commented Sep 5, 2023

If it's allocated to an arena slab, the manual drop is necessary when the arena is dropped. If the value is dropped before then, it's arena tag should be set to ArenaHeaderTag::Dropped.

@triska
Copy link
Contributor

triska commented Sep 6, 2023

@aarroyoc: Very impressive!

One small detail I noticed: Have you thought about naming the HTTPS options tls_cert/1 and tls_key/1? SSL is obsolete, and even Let's Encrypt now only speaks about "TLS certificates". Personally, I would consider "tls..." preferably, we also have library(tls).

src/lib/http/http_server.pl Outdated Show resolved Hide resolved
@triska
Copy link
Contributor

triska commented Sep 13, 2023

One other small thing I noticed: The README still mentions hyper, this (and also the server description, such as the newly available HTTPS support that this patch provides) may benefit from a small adaption:

* [`http/http_server`](src/lib/http/http_server.pl) Runs a HTTP/1.1 and HTTP/2.0 web server. Uses [Hyper](https://hyper.rs) as a backend. Supports some query and form handling.

@aarroyoc aarroyoc force-pushed the warp-http-server branch 3 times, most recently from 0bd94fa to 9d83fc1 Compare September 23, 2023 22:14
@aarroyoc aarroyoc marked this pull request as ready for review September 23, 2023 22:14
@aarroyoc
Copy link
Contributor Author

The last change I wanted to introduce in this PR was allowing the server to be stopped with Ctrl-C. I finally managed to do it, although I'm not 100% happy with this solution, but since POSIX signal hooks are very limited in what they can do, it's ok.

@triska
Copy link
Contributor

triska commented Sep 24, 2023

Awesome, thank you so much! What is the issue with the CI test on windows? (@infogulch ?)

@aarroyoc aarroyoc force-pushed the warp-http-server branch 2 times, most recently from e2e6bd5 to 91348b2 Compare September 24, 2023 12:04
@aarroyoc
Copy link
Contributor Author

About the Windows error: https://users.rust-lang.org/t/gnu-ld-linker-errror-export-ordinal-too-large-xxxxx/84092

@aarroyoc
Copy link
Contributor Author

I'm blocked with this Windows error. Apparently, GNU ld on Windows has this problem (however Scryer itself doesn't export 68000 symbols at all, I've checked pub and pub use without (crate) and I can only count 68000 if we admit dependencies). MSVC toolchain doesn't have this issue and it's free for open source code, so that could be an option. In Rust they're also talking about using LLVM's lld but it's not stable and will go first to the MSVC toolchain. It seems like MSVC toolchain is the most recommended for Windows (rustup recommends you to install this right now).

@infogulch
Copy link
Contributor

infogulch commented Sep 27, 2023

I don't see why we shouldn't switch to msvc, so I'm looking into that.

Separately, I'm curious why this is an issue on this pr in particular, considering that the cause is 'too many symbols'. Looking at Cargo.lock, I see that it adds 20 new dependencies, including rustls, but openssl remains. I'm not sure what to think about that, except that it seems like it would be better to only have one tls library in scryer not two. See also #1531

mthom added a commit that referenced this pull request Sep 27, 2023
Build windows with msvc #1998; add ubuntu 22.04 target #2047
- Use Warp
- Optimize clones
- HTTPS server
- Content-Length limit
- HTTP Basic Auth
- Stop server with Ctrl-C
@mthom
Copy link
Owner

mthom commented Sep 27, 2023

Thanks for removing the block @infogulch ! Is this PR ready to be merged?

@aarroyoc
Copy link
Contributor Author

Thanks @infogulch! Yes, @mthom it's ready. This should be much better than the current implementation.

@infogulch
Copy link
Contributor

Looks good to me.

Maybe we can evaluate the cost/benefit proposition all the dependencies in a future discussion.

@mthom mthom merged commit 5f6ae38 into mthom:master Sep 27, 2023
10 checks passed
@aarroyoc aarroyoc deleted the warp-http-server branch September 27, 2023 20:00
@Skgland
Copy link
Contributor

Skgland commented Aug 3, 2024

is there a reason for pinning warp at 0.3.5? it looks like hyper was pinned as a pre-release version was used and this was just kept for warp.

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.

5 participants