Streamed read for binary HTTP

[martinthomson]

This is far from ready.

It also requires an MSRV update to 1.82, which is brand new, so I don't want to say that this is ready to land.

Missing:

• The writing part. It is likely possible to cobble something together from the synchronous API easily, so I didn't spend too much time on this, but that will have to be worked out.
• Testing. The tests I have are basic smoke tests. I found a lot of missing coverage in a short time.
• ohttp parts. That part should be fairly simple (it can probably reuse a lot of the Body implementation), but this is still likely to be non-trivial.

[imlk0]

@martinthomson Hi Martin,

I've been looking at this PR and have a few questions that I hope you don't mind me asking.

1. I noticed that the PR includes code for reading BHTTP messages from a binary stream using async_read(). Would there be any plans in the future to also support asynchronous writing/conversion of BHTTP messages back into binary streams?

2. Have you considered adding interoperability with the http crate? For example, would it make sense to provide conversions between bhttp::Message and http::Request/http::Response? This could potentially help users who are already working with the standard HTTP types.

3. I saw that the MSRV was raised to 1.82.0 — was this due to a new Rust feature being used? I tried applying a small patch and found that the code seems to compile successfully on Rust 1.75.0 as well. Just curious if lowering the MSRV is something that might be considered in the future.

diff --git a/bhttp/src/lib.rs b/bhttp/src/lib.rs
index 2082b0f..9063e7e 100644
--- a/bhttp/src/lib.rs
+++ b/bhttp/src/lib.rs
@@ -188,7 +188,10 @@ impl FieldSection {
     }
 
     /// Gets all of the values of the named field.
-    pub fn get_all<'a, 'b>(&'a self, n: &'b [u8]) -> impl Iterator<Item = &'a [u8]> + use<'a, 'b> {
+    pub fn get_all<'a, 'b>(&'a self, n: &'b [u8]) -> impl Iterator<Item = &'a [u8]> + 'b
+    where
+        'a: 'b,
+    {
         self.0.iter().filter_map(move |f| {
             if &f.name[..] == n {
                 Some(&f.value[..])

Thanks again for your work on this — it's really helpful!

[martinthomson]

1. I considered it. While this is a pretty obvious thing to do, the writing side of this has far less of a need for this sort of thing. I decided to make that a follow-on. If you have designs on that, I'd be open to working with you on a matching change.
2. That's definitely worth doing as well. Conversions between this and http are something I've been asked about before and it's just been a lack of time that has stopped me from doing the work.
3. The MSRV change is due to dependencies in some of the sundry packages; zerovec specifically, via url. The use<'a, 'b> business is from a slightly earlier release (from memory). As for your change, the extra constraint on 'b might not be right in the general case, but it seems like this can maybe do a check for availability of use<> and engage that as needed. That will make the function more general for those who have the right version of rust, but not constrain those who don't.

[martinthomson]

I've taken your suggested change, though I can't see a way to avoid the 1.82 change, even though it is in a dependency that you might not need. There doesn't seem to be a way to make the MSRV conditional on the feature set chosen. And until we get rust 2024 edition (1.85) we are stuck with a dependency resolver that consistently finds dependencies that can't be compiled.

[imlk0]

@martinthomson Thank you for your detailed explanation and the work you've done on this project.

Regarding the first point about writing BHTTP messages: I'm working on a scenario where I need to receive HTTP messages (e.g. a server-sent event http response), convert them to BHTTP format on-the-fly, and stream them out. This led me to think about implementing an async conversion between bhttp encoded request/response binary stream and http::Request/http::Response as part of the process. Would it make sense to add this as an optional feature to the bhttp crate (e.g., behind a feature = "http-compat") for users who need such interoperability?

I'm happy to draft a PR for this if you think it aligns with the crate's goals.

Looking forward to your thoughts!

[imlk0]

@martinthomson Hi, I have created an early implementation of http-compat #82. Looking forward to your valuable feedback.

[martinthomson]

Oh, this is great. I'll take a look tomorrow.

[imlk0]

Found a panic when decapsulating a long request:

    #[test]
    fn long_request() {
        init();

        // A long request.
        const LONG_REQUEST: &[u8] = &[0u8; 1024];

        let server_config = make_config();
        let server = Server::new(server_config).unwrap();
        let encoded_config = server.config().encode().unwrap();
        trace!("Config: {}", hex::encode(&encoded_config));

        // The client sends a request.
        let client = ClientRequest::from_encoded_config(&encoded_config).unwrap();
        let (mut request_read, request_write) = Pipe::new();
        let mut client_request = client.encapsulate_stream(request_write).unwrap();
        client_request
            .write_all(LONG_REQUEST)
            .sync_resolve()
            .unwrap();
        client_request.close().sync_resolve().unwrap();

        trace!("Request: {}", hex::encode(LONG_REQUEST));
        let enc_request = request_read.sync_read_to_end();
        trace!("Encapsulated Request: {}", hex::encode(&enc_request));

        // The server receives a request.
        let mut server_request = server.decapsulate_stream(&enc_request[..]); // <---- panic here
        assert_eq!(server_request.sync_read_to_end(), LONG_REQUEST);
    }

panic message

---- stream::test::long_request stdout ----
pull output with output.len() = 32

thread 'stream::test::long_request' panicked at ohttp/src/stream.rs:532:19:
range end index 1024 out of range for slice of length 32

The out of range access happens here:

ohttp/ohttp/src/stream.rs

Line 490 in 8628e6c

output[..pt.len()].copy_from_slice(&pt);

[martinthomson]

Well, that embarrassing. That's a pretty big omission on my part. Thanks for finding it, and for the easy test case. The fix is non-trivial, but I'm on it.

[imlk0]

Hey @martinthomson, no worries at all. I'm glad we found the issue!
I also made a quick fix in my branch (here), though I’ll admit it’s not perfect. No pressure to use it, but maybe it can give you some ideas. 😉

[martinthomson]

I've put a fix in place. It should be a little more complete than yours (you lost the value of last), but it's roughly the same shape. Good to see that the general shape of the fix is about the same though.