Skip to content
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

forward the error to the client when upstream closes the connection abruptly #1490

Merged
merged 3 commits into from
Nov 9, 2017

Conversation

i110
Copy link
Contributor

@i110 i110 commented Nov 2, 2017

When the upstream responds a payload whose length is smaller than content-length header value, the current implementation of fastcgi handler and mruby's http_request don't send RST_STREAM frame. This PR makes those handlers send RST_STREAM.

NOTE1: This PR depends on #1489 to make some tests pass

NOTE2: Desirable h2o behaviours

Upstream Payload h2o Payload RST_STREAM (H2) chunked (H1)
smaller than C-L send all received body YES -
bigger than C-L send body upto C-L NO -
TE:chunked ending in mid-of-chunk send all received body YES append a broken chunk (1\r\n)
TE:chunked ending on chunk boundary but no eos send all received body NO append an eos (0\r\n)

SEE ALSO: #1031

@kazuho
Copy link
Member

kazuho commented Nov 3, 2017

Thank you for the fix and also for the chart. The chart gives us a good understanding on how we forward errors.

That said, the chart raises one concern: when upstream closes the connection mid-chunk and downstream is h1, we try to forward error by omitting the eos (i.e 0-cr-lf-cr-lf).

Is that the correct thing to do?

My understanding is that the lack of eos is not handled as an abrupt close by some of the browsers (see the discussion on #1031, that's why we changed our http1client code in how we detect errors).

Do we need to use some other way onto propagate the error to the client, for example by closing the connection after sending 1-cr-lf-cr-lf so that it would look like a mid-chunk close?

cc @deweerdt

@deweerdt
Copy link
Member

deweerdt commented Nov 8, 2017

@i110 thanks for the bugfix and explanation. I'm curious what the motivation for sending RST_STREAM is? Are you expecting the client to react in a more visible way (e.g. broken images, rather than partially rendered ones), making issues easier to detect?
I'm a bit worried that applying this to proxying will suddenly break some previously ok traffic (since the client might rely entirely on the H2 framing, rather than on CL), what do you think about that use case?

@i110
Copy link
Contributor Author

i110 commented Nov 8, 2017

@kazuho

Do we need to use some other way onto propagate the error to the client, for example by closing the connection after sending 1-cr-lf-cr-lf so that it would look like a mid-chunk close?

This code does it. I edited the chart to describe that.

@deweerdt
Yes, I think the client should be able to know as explicit as possible whether the response is broken or not. And also, in current implementation the proxy handler sends RST_STREAM in such case, so it might be better even in terms of consistency to have mruby and fastcgi handlers do the same thing too. Just so you know, this PR doesn't change the behaviour of the proxy handler at all.

@kazuho
Copy link
Member

kazuho commented Nov 9, 2017

@i110

this PR doesn't change the behaviour of the proxy handler at all.

Yes. This PR does (at the moment) focuses on fixing issues specific to fastcgi and mruby handlers.

@deweerdt That said, what I wanted to ask is if our behavior when forwarding a chunked response from upstream server to a H1 client is fine. I asked here because the issue is related. Sorry for the confusion.

As I described in #1490 (comment), we consider that a response has closed without an error a) when we observe 0-cr-lf-0-cr-lf (as defined in the RFC 7230), or b) when the connection is being closed at a chunk boundary. We consider b as a successful close because browsers work that way (see #1031 (comment)).

And we try to propagate the condition (i.e. if H2O has successfully received all response) to the client. That has been the purpose of #1031, and I am fine with that.

However, I am afraid that we are failing to propagate the error to a HTTP/1 client, because we are using the existence of 0-cr-lf-cr-lf as the signal to the client. As stated above, that will not be handled as an error by some browsers.

Do we need to change the behavior?

@deweerdt
Copy link
Member

deweerdt commented Nov 9, 2017

However, I am afraid that we are failing to propagate the error to a HTTP/1 client, because we are using the existence of 0-cr-lf-cr-lf as the signal to the client. As stated above, that will not be handled as an error by some browsers.

Ha, thanks for pointing this out. I'm not entirely sure. I think it would make sense to look at what other servers do before doing this change? Intuitively, i agree that it would be better to explicitly signal to the client that the transfer was broken.

@i110
Copy link
Contributor Author

i110 commented Nov 9, 2017

Hm? I believe that

for example by closing the connection after sending 1-cr-lf-cr-lf so that it would look like a mid-chunk close?

This is already done in here as I mentioned in #1490 (comment). Do you mean that this is wrong or not enough way to propagate errors to H1 client?

@i110
Copy link
Contributor Author

i110 commented Nov 9, 2017

I think it would make sense to look at what other servers do before doing this change?

I tried nginx and it doesn't care such case at all. If the upstream closes the connection in mid-of-chunk, nginx responds as-is, without appending any eos or broken chunks.

@kazuho
Copy link
Member

kazuho commented Nov 9, 2017

This is already done in here as I mentioned in #1490 (comment). Do you mean that this is wrong or not enough way to propagate errors to H1 client?

@i110 @deweerdt Oh! I missed that. If that is the case, we are doing things correctly. Sorry for the fuss.

@kazuho kazuho merged commit 89863f5 into master Nov 9, 2017
@kazuho kazuho changed the title send RST_STREAM when received smaller payload than content-length header forward the error to the client when upstream closes the connection abruptly Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants