-
Notifications
You must be signed in to change notification settings - Fork 91
Make UntypedBody be able to extract to a BufList
#542
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
Make UntypedBody be able to extract to a BufList
#542
Conversation
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Created using spr 1.3.4
|
It occurred to me that we could make In a followup we could then switch the current |
Created using spr 1.3.4
|
I switched over to |
Created using spr 1.3.4 [skip ci]
davepacheco
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.
Sorry -- I'd written most of this up before your latest change.
| #[derive(Debug)] | ||
| pub struct UntypedBody { | ||
| content: Bytes, | ||
| request: Arc<Mutex<Request<Body>>>, |
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.
Is it okay to take this lock at the point where we take it? Previously, we read this whole thing earlier. I get why we don't do this now, but that means we're taking a lock later in the request processing. I thought we'd be holding the lock for longer, too, but I don't think that's true.
dropshot/src/http_util.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// Errors if the body length exceeds the given cap. | ||
| pub async fn http_read_body_bytes<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.
Should we just use BufList internally and only turn that into a String (or Bytes I guess) if we really need it? I think the main use case where we want a String is because we're going to parse it with serde. Is it a lot more efficient to read it to a Bytes first than a BufList?
Created using spr 1.3.4
| let mut request = self.request.lock().await; | ||
| let body = request.body_mut(); | ||
|
|
||
| 'outer: { |
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.
I know I said I'd reserve my feedback until you go through @davepacheco's comments, but free advice on this: it might be less work to defer the use of relatively-new structures rather than potentially impacting some dropshot consumer at Oxide that's on an older rust version. My personal threshold is about 4-6 months in terms of my willingness to just hope that folks are up-to-date.
|
Discussing this with @davepacheco and @ahl, will mark this as draft until we come to a consensus. |
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
|
should we close this PR? |
|
Yes, will re-do this per RFD 353 later. |
|
(re-did in #617) |
(This is a version of #541 that breaks BC, per Adam's suggestion. I
do agree that this is overall a better approach.)
The current
UntypedBodyextractor writes data into a singleVec<u8>.Consider what happens if the body is large (e.g. 100MB, which can happen
if uploading an artifact over HTTP). As each chunk (typically 10-100KB)
comes in, we'll have to both copy data from the incoming
Bytes, andreallocate the
Vecover and over.To avoid this issue, Eliza Weisman and I wrote buf-list, which
represents a list (really a queue) of chunks that can be operated on
using standard Tokio and other abstractions:
https://crates.io/crates/buf-list.
Make the
UntypedBodyextractor represent a bytestream that hasn't beenread yet. This allows us to extract the body as a
Bytes, aBufList,or any other stream one chooses.
One other change I did is to remove the nonexistent type parameter
Jfrom
UntypedBody<J>suggestions -- that didn't look right.One consideration here is that
BufListneeds to be exposed as a type.It's currently at 0.1 -- I could release a 1.0 if that would be helpful
as far as exposing in the API goes. What do you think?