Change 100-continue behavior to send when Body has been polled

[bozaro]

Use of the 100 (Continue) Status (https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3):

The purpose of the 100 (Continue) status (see section 10.1.1) is to allow a client that is sending a request message with a request body to determine if the origin server is willing to accept the request (based on the request headers) before the client sends the request body. In some cases, it might either be inappropriate or highly inefficient for the client to send the body if the server will reject the message without looking at the body.

In my case, for example, I can upload file or reject already uploaded file in one atomic request.

Current state:

• 0.9.x - handler can make desigion and allow/reject client request by additional check_continue method. This method is not supported by https://github.com/iron/iron and https://github.com/nickel-org/nickel.rs. Also this method require duplicate logic (it not called on HTTP/1.0) for routing and check permissions;
• master - looks like this feature is forgotten.

I think, much better to send "100 Continue" status on first Body::read() call (similar logic implemented by https://github.com/frewsxcv/tiny-http project).

In this case no additional logic/callback needed for make desigion about "100 Continue":

• If you call Body::read() method, then you needs to request body and hyper send to client "100 Continue";
• If you have anought information to send final status without body, then hyper send status without "100 Continue".

I try to change logic on 0.9.x branch by send "100 Continue" status of first read call, but I failed :(

[seanmonstar]

master - looks like this feature is forgotten.

Not so much forgotten, I just wanted to come up with a better design. What you outline here seems like it should work on master:

fn on_request(&mut self, req: Request) -> Next {
    match request.headers().get() {
        Some(&ContentLength(len)) if len > 10_000_000 => {
            self.status = StatusCode::PayloadTooLarge;
            Next::write()
        },
        None => {
            self.status = StatusCode::LengthRequired;
            Next::write()
        },
        Some(&ContentLength(len)) => {
            self.length = len;
            // hyper looks for Expect-100, and sends 100 status because of this read()
            Next::read()
        }
    }
}

[bozaro]

Yes, about this behavior and I had in mind.

I'm sorry if offended. Loss (sometimes temporary) features on full code redesign is a normal and I did not mean anything bad. On the contrary, from my point of view this is a very good time to remind about corner cases :)

[seanmonstar]

Not offended at all! Thanks for helping think this issue through.

On Mon, Jun 20, 2016, 11:42 PM Artem V. Navrotskiy [email protected]
wrote:

Yes, about this behavior and I had in mind.

I'm sorry if offended. Loss (sometimes temporary) features on full code
redesign is a normal and I did not mean anything bad. On the contrary, from
my point of view this is a very good time to remind about corner cases :)

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#838 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AADJF2kg53rTezjlZGbnHTCzmF2ekBXDks5qN4ffgaJpZM4I5p6B
.

[martell]

Hey all,
I'm looking at implementing this on master with the tokio integration.

I've literally only been using rust a few days so it is hard for me to follow through the code with inferred type assignments and generics mixed together.

Anyway, Here is what I have so far.

diff --git a/src/http/conn.rs b/src/http/conn.rs
index 7954fadb..3e90c8ba 100644
--- a/src/http/conn.rs
+++ b/src/http/conn.rs
@@ -116,6 +116,7 @@ impl<I: Io, T: Http1Transaction, K: KeepAlive> Conn<I, T, K> {
                     }
                 };
                 self.state.busy();
+                let wants_continue = head.expecting_continue();
                 let wants_keep_alive = head.should_keep_alive();
                 self.state.keep_alive &= wants_keep_alive;
                 let (body, reading) = if decoder.is_eof() {
@@ -124,6 +125,10 @@ impl<I: Io, T: Http1Transaction, K: KeepAlive> Conn<I, T, K> {
                     (true, Reading::Body(decoder))
                 };
                 self.state.reading = reading;
+                if wants_continue {
+                    self.state.reading = Reading::Init;
+                }
+
                 return Ok(Async::Ready(Some(Frame::Message { message: head, body: body })));
             },
             _ => {
@@ -674,6 +679,8 @@ mod tests {
             Ok(())
         }).wait();
     }
+
+
     #[test]
     fn test_conn_closed_write() {
         let io = AsyncIo::new_buf(vec![], 0);
diff --git a/src/http/mod.rs b/src/http/mod.rs
index 13c50119..a0ea5a27 100644
--- a/src/http/mod.rs
+++ b/src/http/mod.rs
@@ -2,7 +2,7 @@
 use std::borrow::Cow;
 use std::fmt;
 
-use header::{Connection, ConnectionOption};
+use header::{Connection, ConnectionOption, Expect};
 use header::Headers;
 use method::Method;
 use status::StatusCode;
@@ -68,6 +68,10 @@ impl<S> MessageHead<S> {
     pub fn should_keep_alive(&self) -> bool {
         should_keep_alive(self.version, &self.headers)
     }
+
+    pub fn expecting_continue(&self) -> bool {
+        expecting_continue(self.version, &self.headers)
+    }
 }
 
 /// The raw status code and reason-phrase.
@@ -115,6 +119,16 @@ pub fn should_keep_alive(version: HttpVersion, headers: &Headers) -> bool {
     ret
 }
 
+#[inline]
+pub fn expecting_continue(version: HttpVersion, headers: &Headers) -> bool {
+    let ret = match (version, headers.get::<Expect>()) {
+        (Http11, Some(expect)) if expect == &Expect::Continue => true,
+        _ => false
+    };
+    trace!("expecting_continue(version={:?}, header={:?}) = {:?}", version, headers.get::<Expect>(), ret);
+    ret
+}
+
 #[derive(Debug)]
 pub enum ServerTransaction {}
 

The function I added to http/mod.rs seems correct and with RUST_LOG=trace I get

TRACE:hyper::http: expecting_continue(version=Http11, header=Some(Continue)) = true

Ignoring the changes I made to conn.rs just for testing.

@seanmonstar can you give me some direction here?
I can gather that we would be using Sink and not a Stream for this but I am not sure about what would be the best way to go about integrating this into the code base.
Where should I be piping the data back etc.
I'm am assuming this is a good time for api breakages.

[seanmonstar]

@martell You've only been using Rust a couple days? That makes this work even more impressive! I'd suggest opening a Pull Request with your work, it makes it easier to discuss changes.

As for your current diff, it definitely looks like the right direction. I imagine we would want to add some new states, of which I'm not entirely certain the best combination at the moment. I'll think more, but I can describe the expected behavior.

We would need to save somehow that the request would like a 100-continue response. We would also need a way to know whether we should respond with it, and also to know that we already have. A way we could decide to write 100-continue is if the user decides that they wish to keep reading from the body, we should flush that response. If the user decides they don't want to read from the body, they'll just return a Response, and we can just write that.

if !req.headers().has::<ContentLength>()
    return Response::new().with_status(StatusCode::LengthRequired);
} else {
    req.body().for_each(|chunk| something(chunk))
}

In the above example, if the request didn't have a Content-Length header, the server wants to reject it immediately. In this case, hyper should not have responded with 100-continue. We can do this because Conn::poll() will be called again if the user tries to poll on the request body (the req.body().for_each).

So, we can record that an expectation exists, and then in Conn::poll(), if were in this expecting state, we can write (and it needs to be flushed immediately) the 100-continue response (record that we've since written that), and then try to keep reading the body. If Conn::start_send() is called to write a response, we can just remove the expectation state, since it no longer matters.

Does this all make sense?

[martell]

Thanks for taking the time to look.

Most of that makes sense to me.
I have setup a WIP PR for this

We have to be compliant with the rfc spec to only send the 100-continue when we get the Content-Length makes sense to me.
Where does the above code go in the code base however?

The use case for this is typically for a REST API server and we only really want to send the body after Auth has happened based on the header data, probably an oauth2 bearer token etc.
So when the user requests the body is an ideal time to send the 100-continue

I see the Conn::poll() in conn.rs
I'm just not sure what states yet either but can play around with that to achieve the goal and refactor, from initial reading I see

Reading::Init
Reading::Body(..) in the function `can_read_body`
Reading::KeepAlive
Reading::Close

I think we want a state like Reading::Head because we are beyond the init stage but are not onto reading the body yet.

My biggest problem is I can't find where I can write to the SINK, this is probably a combination of my lack of rust knowledge and the fact that this is not my own code base :)

[seanmonstar]

I've gone back and forth whether it should be Reading::Expect or Writing::Expect. It kind of affects both. Whichever is chosen, we'd want to update read_body to check that we've written the 100-continue, and write_head would want to update the state that that there is no need to send a 100-continue, since some other response is already being sent.

The content is written via self.io.buffer(bytes) and then flushed with self.io.flush().

[sfackler]

I have a PR open to implement this: #1232.

[martell]

@sfackler Added support for immediately replying with 100-continue
This is a good short term fix for getting support in tree.
Ideally we would only send this on the attempted read of the body.

Discussions on IRC lead to some work being done in tokio which needed a new future Sink to support this. rust-lang/futures-rs#414
This was then dropped afterwards.

It is still up in the air about how we are going to best solve this.
I wanted to get some context into this thread so we can proceed.
@seanmonstar did mention he might have an idea about how to help this in hyper?

This thread might give the best context on what is needed.
rust-lang/futures-rs#409

[seanmonstar]

This should no longer be blocked on any dependencies, on master. We have our own dispatcher, and use a custom channel for the body.

I think it wouldn't be too hard to tie want into the body channel, to know when and if the user has polled for data on the body.

[AlexDvorak]

@seanmonstar was this not fixed by 26417fc?

if msg.expect_continue {
   let cont = b"HTTP/1.1 100 Continue\r\n\r\n";
   self.io.headers_buf().extend_from_slice(cont);
}

[seanmonstar]

@AlexDvorak no, that's adding the 100 Continue response to the write buffer immediately, right after parsing. This issue is describing that it shouldn't add it immediately, but instead only if the user polls the Body. That way, if the user rejects the request for some reason, the client doesn't see a weird HTTP/1.1 100 Continue\r\n\r\nHTTP/1.1 400 Bad Request.

[AlexDvorak]

So, would modification to the Body struct like so be fine?

pub struct Body {
    kind: Kind,
    /// Keep the extra bits in an `Option<Box<Extra>>`, so that
    /// Body stays small in the common case (no extras needed).
    extra: Option<Box<Extra>>,
    UserHasPolled: bool,
}

Or should the modification be happenning elsewhere?
If that's ok then how would one access the Body from the proto/h1/conn.rs?

[seanmonstar]

That wouldn't work, since the Conn no longer has the Body side. When there's a request body, a channel is made (Body::channel). The channel implementation could be where this information is stored. The client::dispatcher already has a concept of knowing when the receiver side has been polled, by making use of want.

[AlexDvorak]

what are you reffering to by channel implementation? I assume your're not reffering to mpsc::channel