fix(client): drop connection if body of a response was not consumed, to avoid bad state

[joelwurtz]

Pr is not finished, i only added a test.

Actually if a body is not readed on the client, but the reponse is dropped, conn will go back to the pool with existing data of the response not consumed, which will fail the next request since it will try to decode the header on the previous response body

This may happens if a client send a request, gets an error with a status code and a message in the body by example, and when there is a specific status code (in this case the error), will not bother to consume the body.

Not sure how to handle this well, i assume we should force the body to be consumed before going back to the pool but i'm not sure how to do this

[joelwurtz]

Not sure this is best way, but at least it's safer, i'm not sure how to consume the body if dropped so for the moment i destroy the connection in this case.

I think it will require a specific state of the connection so it will ready the previous body before the next request in this case, but maybe you will have a better feedback for this.

[fakeshadow]

I believe using TransferCoding::is_eof as the condition for destroy connection on drop would be enough.

In xitca-http a body with content-length: 0 header would be converted to TransferCoding::Eof in place(Not exactly the described process in real but very close) to hint user can avoid polling beforehand. And there should be no need to handle it specially. Related code:

xitca-web/http/src/h1/proto/codec.rs

Line 252 in 99f7c97

// skip set when the request body is zero length.

As for body with transfer-encoding: chunked I feel the current code be exposed as not handling eof properly. (The body should explicitly branch on ChunkResult::OnEof by polling the stream one more time recursively and enter ChunkResult::AlreadyEof branch). Something like this should be enough:

impl<C> Stream for ResponseBody<C> {
    type Item = Result<Bytes, BodyError>;

    fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
        let this = self.get_mut();

        loop {
            match this.decoder.decode(&mut this.buf) {
                ChunkResult::OnEof => {
                    // this does not really impact the state of TransferCoding other than hint who polling the ResponseBody should not skip the OnEof state.
                },
            }
        }
    }
}

[fakeshadow]

There could be edge case where user weirdly decide to ignore body state and stop polling the stream when OnEof state appear. But I personally don't believe this is case we should burden ourselves to support. The worst case is connection can be closed prematurely due to user not polling body until it reaches None state.

[joelwurtz]

I updated the PR to use is_eof instead

[fakeshadow]

Sorry I forgot to mention that it could be better to impl the Drop on h1::body::ResponseBody to reduce feature gated code. The change and test are good.

[joelwurtz]

It's not possible to drop it on h1::body::ResponseBody due to the generic data type i cannot enforce a drop on every possible one (but given how it's used we could now drop this generic data type since there is no difference between owned and not owned anymore, if you are fine with that i can make the change)

[fakeshadow]

Yea it's Ok to remove generic type from the h1::body::ResponseBody and use connection::H1ConnectionWithKey as concrete type. body::ResponseBody::H1Owned variant can be completely remove to make this happen. They are mostly legacy artifact right now.

[joelwurtz]

Should be good then, clippy failure seems unrelated

[fakeshadow]

Looks good