-
Notifications
You must be signed in to change notification settings - Fork 37
http-compat: add support for convert between bhttp binary and http crate #82
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
Conversation
martinthomson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some preliminary feedback. I find myself short on time today (as always), so it's not as in-depth as I'd like.
bhttp/src/http_compat/decode.rs
Outdated
| /// | ||
| /// This is the final result of the decoding process, containing either | ||
| /// a fully constructed HTTP request or response with a streaming body. | ||
| pub enum HttpRequestOrResponse<R> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub enum HttpRequestOrResponse<R> { | |
| pub enum HttpMessage<R> { |
How do you imagine someone using this type? Do they simply call HttpMessage::from(...)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name HttpMessage seems clearer than before.
However, I don't think it makes sense to allow users to directly call HttpMessage::from(...) because that would require a BhttpBody, which should only be created through BhttpDecoder::decode_message(). To enforce this, I will make BhttpBody::new() private in a later commit.
bhttp/src/http_compat/encode.rs
Outdated
|
|
||
| /// Enum to wrap either an HTTP Request or Response for unified BHTTP encoding | ||
| #[derive(Debug)] | ||
| enum HttpRequestResponse<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| enum HttpRequestResponse<T> { | |
| enum HttpMessage<T> { |
So the trick here is that you wrap and then implement functions to encode those messages into the underlying type. And T is the body type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the http crate uses a generic type T to represent the body type, and as long as T implements http_body::Body, I think this is a great design choice. Perhaps in the future we could refactor bhttp::Message into bhttp::Message<T>, which would allow us to avoid using content: Vec<u8> internally for storing content.
Lines 592 to 598 in c6131ac
| pub struct Message { | |
| informational: Vec<InformationalResponse>, | |
| control: ControlData, | |
| header: FieldSection, | |
| content: Vec<u8>, | |
| trailer: FieldSection, | |
| } |
bhttp/src/http_compat/encode.rs
Outdated
|
|
||
| #[test] | ||
| fn test_encode_rfc9292_request_example_indeterminate_length() { | ||
| // Modifyed example from Section 5.1 of RFC 9292 - Indeterminate-Length Binary Encoding of Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Modifyed example from Section 5.1 of RFC 9292 - Indeterminate-Length Binary Encoding of Request | |
| // Modified example from Section 5.1 of RFC 9292 - Indeterminate-Length Binary Encoding of Request |
How modified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original bhttp example, the scheme is https, the authority is empty, and the path is /hello.txt. However, this is not allowed by the http crate (see source), which requires that if a scheme is present, the authority must also be present.
To make the test meaningful, I have modified the bhttp example to set a valid authority, ensuring it conforms to the HTTP URI specification.
I will add a comment describing the change later.
| assert!(!bhttp_data.is_empty()); | ||
|
|
||
| // The data should be longer than just the body since it includes headers and metadata | ||
| assert!(bhttp_data.len() > 9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty weak in terms of its utility as a test. I suggest that you have a set of known-answer tests for these. On the one hand, http::Request, on the other a byte array. Those can live in a common test module. This code can run down the list and check that the output matches the expectation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great suggestion. It would make test expansion much more convenient. Do you have any ideas about geting more of such combinations? There are some in RFC 9292, but I've already used them all.
|
Thank you for your feedback. I will update this PR later :) |
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
…HeaderMap Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
…ering a trailers frame Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
|
Updated based on your feedback and marked most reviews as resolved. Left a few open for further discussion, would appreciate your thoughts! |
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
…onsuming Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
…and http Signed-off-by: Kun Lai <laikun@linux.alibaba.com>
|
@imlk0, I'm sorry, but I didn't realize that merging the other PR would close this. And I can't reopen it. Can you do that? I think it needs a new pull request. (I'm still considering taking this code; it's good, but I need to do a bit more testing on my own to get more confident in it.) |
|
Sorry for the long delay in getting back to this. I’ve reopened the PR and rebased it onto the latest Based on further testing and use in my side, I’ve incorporated several improvements to enhance stability and functionality:
I’ve been using this branch in my own project to encrypt both HTTP requests and responses (including SSE streaming) for vLLM inference, and it’s been working well. I’m still committed to helping move this forward. please let me know if you'd like any further adjustments or have additional feedback :). Thanks! |
This PR introduces a new
http-compatmodule that provides asynchronous streaming conversion between HTTP types from thehttpcrate and BHTTP format. This enhancement allows seamless conversion between standard HTTP requests/responses and BHTTP binary format using asynchronous streams.Note: this pull request is based on #74
Changes
BhttpEncoderstruct for convertinghttp::Request/http::Responseto BHTTP data stream (futures::prelude::Stream<Item = Res<Vec<u8>>>)BhttpDecoderstruct for converting BHTTP binary data (futures::io::AsyncRead) back tohttp::Request/http::Responsehttp-compatfeature flag that enables this featureTesting
cargo test --features http-compat http_compatUsage
With this change, users can now easily convert between standard HTTP types and BHTTP format: