Short requests/responses could use more granular exceptions.

[Lukasa]

When either the server or the client sends a HTTP message with a body that is short (that is, the body does not contain enough bytes to meet the Content-Length header, or the chunked encoding is aborted without the ending), an exception like this is raised:

RemoteProtocolError("can't handle event type <class 'h11._events.ConnectionClosed'> for SERVER in state SEND_BODY",)

This message, while understandable, is a little hard to introspect. It would be nice if we could have a subclass of the ProtocolError that indicated exactly the kind of error, so that applications that would like to report these errors in a more granular way could do so.

[njsmith]

Sure, it's hard to object to finer-grained errors :-).

What exactly do you want to detect? is it specifically: any time the state machine errors out due to an unexpected event, where that event is of type ConnectionClosed? Do you also want this if they drop the connection when, say, they're half-way through the headers?

[njsmith]

(NB I just also just tweaked that error message slightly in master, because <class h11._events.ConnectionClosed> was bugging me now that you pointed it out :-))

[Lukasa]

@njsmith Something like that, yeah. Basically, I'd like to be able to reliably detect violations in HTTP framing that have to do with the body: whether the body is too short or too long.

Halfway through the headers bothers me less right now, though I may come back to it at some point. This is part of my work on urllib3, which has historically thrown specific errors for bodies that are too short/too long.

[njsmith]

Oh, I see, right, that's a little different.

How do you detect bodies that are too long? Right now I think h11 will just buffer any "extra" bytes and treat them as part of the next request/response cycle...

For too short, it sounds like you specifically want to detect ConnectionClosed when in state SEND_BODY. As a wrinkle, though, note that if a chunked-encoded connection gets dropped, we aren't actually guaranteed to get that specific exception you noticed, with the invalid state transition. We only detect that state transition when the parser reaches the EOF, and that only happens if we successfully parse all the data leading up to the EOF. If the connection drops half-way through a chunk header (like the last few bytes are \r\n100\r then EOF), then we instead hit this branch, which detects the generic condition "the parser is waiting for more data but the connection is closed".

Some possible approaches:

• Have a specific "connection closed while reading body" exception. This seems a little oddly specific?

• Tag the invalid state transition exception with those details of what the event and state were; then your code can check for those attributes. This would need a bit of finesse in handling the LocalProtocolError/RemoteProtocolError hand-off, though that's not too big a deal. It doesn't handle the truncated-chunk-header issue.

• In your code, check for the following sequence:

  • conn.next_event() returns NEED_DATA
  • and conn.their_state is SEND_BODY
  • then we call conn.receive_bytes(b"")
  • then we call next_event() and it raises a RemoteProtocolError

I think this would actually work, though it's a bit awkward. (I guess depending on the structure of your code, the first two conditions might be implicit -- if you only call recv after getting a NEED_DATA then that's handled automatically, and SEND_BODY just means you're in between the Response and the EndOfMessage.)

Thoughts?

[Lukasa]

How do you detect bodies that are too long? Right now I think h11 will just buffer any "extra" bytes and treat them as part of the next request/response cycle...

Well, in principle one does that by checking whether or not there are further requests outstanding to which those bytes could belong.

However, you raise a good point: the way I've written the urllib3 plugin means that it's basically not possible to detect overlong bodies except by luck: if I feed h11 more bytes than the response has left then h11 can see it, but there's always a chance I'll get it exactly right and h11 won't notice.

So let's reduce the requirement on overlong bodies and say just short ones for now.

As you describe all the wrinkles with this, it occurs to me that we might be better served trying to handle this at the urllib3 level. If we keep track of how many bytes we've read and how many we're expecting, we can at least correctly identify bodies that are too short. That won't help us with bad chunked bodies yet, but I can burn that bridge when I get there.

[njsmith]

Well, in principle one does that by checking whether or not there are further requests outstanding to which those bytes could belong.

Well, this would be a good excuse, except that h11 in client mode refuses to let you pipeline :-).

As you describe all the wrinkles with this, it occurs to me that we might be better served trying to handle this at the urllib3 level. If we keep track of how many bytes we've read and how many we're expecting, we can at least correctly identify bodies that are too short. That won't help us with bad chunked bodies yet, but I can burn that bridge when I get there.

Is parsing Content-Length really simpler than tracking whether you just fed h11 an EOF? Parsing Content-Length is actually really non-trivial.

[njsmith]

(Though h11 does at least validate that there is only 1 Content-Length field and that it looks like a number, so you don't have to worry about that)

[Lukasa]

Well, this would be a good excuse, except that h11 in client mode refuses to let you pipeline :-).

Sure, but that just makes it easier: one would assume that any further bytes from the server after EndOfMessage are in violation of the spec until we've sent a new Request.

Is parsing Content-Length really simpler than tracking whether you just fed h11 an EOF?

We don't really have an option here. We don't have to keep track of whether we fed an EOF, but we do have to keep track of whether that EOF was expected or not, and expected in this context is defined almost entirely by the content length. So if h11 can't tell us, that means we have no option but to do it ourselves. In particular, we have historically raised exceptions that say how many bytes we were expecting and how many we got: to correctly extract that data we need either urllib3 or h11 to keep track of that information.

I should note that h2 does keep track of this information itself, and errors in both cases. Of course, h2 has an easier time of detecting bodies that are too long, but too short is usually pretty easy.

[njsmith]

We don't have to keep track of whether we fed an EOF, but we do have to keep track of whether that EOF was expected or not

The suggestion was that if you have an h11.Connection that isn't raising an error, and then you feed it an EOF, and now it raises an error... then that EOF was not expected :-)

[njsmith]

I guess we could put a flag onto protocol errors saying "was this triggered by an unexpected EOF yes/no". Seems like a pretty weird API, but I guess it wouldn't be technically difficult. We can make h11 be helpful, I'm just not sure what helpful looks like.

[Lukasa]

@njsmith Right, but "unexpected EOF" is not the same as "short body". This is what I'm getting at here: urllib3 can detect "unexpected EOF", but it cannot detect why the EOF was unexpected without keeping track of content lengths. And once it starts doing that, we may as well just avoid doing the work in h11 and instead do it in urllib3.

[njsmith]

"unexpected EOF while reading body" is the same as "short body", isn't is? Am I missing something?

[njsmith]

(btw, I'm assuming but maybe should confirm: the issue here is that you want to treat connection drop in the middle of a chunked response as different from a parse error inside the chunked response -- e.g. a chunk with length q -- right? because if all you really cared about is knowing whether the body was received successfully or not then life would be much simpler :-))

[Lukasa]

"unexpected EOF while reading body" is the same as "short body", isn't is? Am I missing something?

Not necessarily, it may be "malformed chunked body", which we'd like to report differently. Complaining about "expected body of length n, received body of length m" makes little sense when we're working with chunked bodies.

Essentially all I'm trying to ascertain is whether it is easier for h11 to raise exceptions that distinguish these two failure modes than it is to just say that callers should extract the relevant information from h11 if they care. Either is a reasonable answer, but there is no getting around the idea that the caller needs to either be told by h11 or keep track of appropriate state itself to understand why.

[Lukasa]

And the reason I've been suggesting h11 do this is only that it is presumably already tracking this information, so it's just a matter of surfacing it.

[njsmith]

Ohhhh, I thought you wanted a truncated chunked response to count as "short body"

[Lukasa]

@njsmith Well, if you wanted to you could argue that there are really two cases there. The first is malformed chunks (that is, any parsing logic that doesn't end up in the EXPECTING_NEXT_CHUNK state, such as a truncated chunk header or chunk body), and the second is a short chunk encoding (EOF after a complete chunk, no 0\r\n\r\n) completion marker.

However, that level of granularity isn't necessarily worth surfacing. But urllib3 has historically treated those two failure modes differently.

[njsmith]

So this is another of those "seemed like a good idea at the time" things then, right? I can't actually think of any good reason why a client would want to distinguish between "I got only a partial response because the connection dropped unexpectedly, and I know how long the body would have been if I'd gotten all of it" versus "I got only a partial response because the connection dropped unexpectedly, and I don't know how long the body would have been if I'd gotten all of it"?

I thought you wanted to distinguish between "protocol stream is malformed due to EOF" and "protocol stream is malformed due to actual bytes", e.g. a chunk header that contains non-hex characters.

[Lukasa]

@njsmith This is a purely error-reporting concern: giving users enough information to understand what's happening in their logs. Generally speaking it's nicer to be granular even if there is very little automated action that can be taken on the distinction, if only so that a user can contact the server-author and go "why the hell are you reporting content length wrong?"

[njsmith]

If it's precisely and exactly that you want to detect the case of a length-delimited message body that gets an unexpected EOF, then h11 does have that information, yeah. You could add a read_eof() method to h11._readers.ContentLengthReader that raises some unique exception, and that would get called exactly in this case (grep for read_eof in _connection.py and _readers.py for context).

[Lukasa]

Yeah, so in an ideal world h11 would also tell me how much it read and how much it expected to read, where I presume again that h11 is keeping track of both of these things. Are you amenable to having that change made?

[njsmith]

Technically right now it's only keeping track of how many bytes it thinks are still outstanding, but that's trivial to change.

I definitely don't have any objection to adding a read_eof method to ContentLengthReader that raises a vanilla RemoteProtocolError with a nice human-readable message explaining the problem.

I hesitate a little about creating a "computer-readable" API for this (e.g. a special ConnectionLostWithIncompleteLengthFramedBody exception with bytes_read, bytes_expected fields or whatever). It's probably fine, but I'd feel better if I understood more details. Let me brain dump a little.

One non-issue is the thing I raised about LocalProtocolError versus RemoteProtocolError above: for error-checking in generic code like the state machine there is some cleverness required here, but for error checking inside the protocol parsers and next_event then there's no problem, because that code only runs on "remote" data. So technically raising ConnectionLostWithIncompleteLengthFramedBody would be easy to implement.

I originally thought the motivation here was the desire to distinguish between connection drops -- which can happen because of bad network weather, or a server VM crash, or whatever, and so a client might want to be somewhat tolerant and retry -- versus a protocol error due to the server doing something actively illegal, where the only solution is to open twitter and send confused emoji faces at the server author. That still seems like it might be a useful distinction to make? Would adding this weirdly-specific exception class interfere with more sensible exception classes in the future?

Also, now it sounds like you're saying the goal is just to have a nice human-readable message in the logs, for which a vanilla RemoteProtocolError would be sufficient. My impression though is that even if this is the actual use case, urllib3's API has committed to providing a bit more than this, i.e., it actually does have a computer-readable special exception type? OTOH this is probably a little different from the usual kind of API commitment, right, in that if urllib3 just stopped raising that special exception type then probably no-one would actually notice? We don't think there's anyone out there who's relying on the ability to truncate a length-framed message body and then catch that exception to do something special?

Okay, now I'll go look at the code to try and understand better what exactly the commitments here are...

Hmm, it looks like the whole reason IncompleteRead exists in the first place is because of httplib API weirdness, where on an incomplete read it actually needs a special exception class so that it has a way to pass back the data that was read? Then urllib3 goes out of its way to throw that data away again because returning values via exceptions is terrible?

Also, it looks like right now httplib when doing chunked reads will throw IncompleteRead with nonsensical values (because it treats each chunk like its own mini-body?). And also if you call httplib.HTTPResponse.read with a non-None amt argument, and it gets an unexpected EOF, then it just silently treats that like a successful body read? OMG this is terrible.

Ok, moving on to urllib3.

Oh phew. At first I thought the silently-allowing-truncated-bodies thing was actually still there in urllib3, but after a git pull it looks like urllib3 has started working around that. git blame says this is thanks to some other Nate, and happened...uh... 4 months ago? At least if the user sets enforce_content_length=True, which isn't the default?

I think at this point I have like some legally mandated requirement to inform you that my professional recommendation is that you set httplib on fire and never use it again. Also does this mean you're going to want an enforce_content_length=False option in h11? Because that's going to be messy...

Anyway, regarding IncompleteRead: urllib3 appears to use httplib's _read_chunked some of the time, and other times it uses its own _handle_chunked code, which in turn calls httplib's _safe_read. So both pieces of code have the same issue, where if the connection drops then they raise an IncompleteRead error with flat-out-wrong metadata. Also, it looks like currently the way both of these methods react to flat-out-invalid data from the server, like a chunk header that isn't valid hex, is by raising IncompleteRead.

So AFAICT the actual current rule is that urllib3 will raise IncompleteRead on any error that occurs while reading the body, regardless of whether it's due to a connection drop and regardless of what kind of framing is being used, and it will attach some metadata that might or might not mean anything.

Hey, it sounds like maybe h11 can emulate that API pretty well already? :-)

[Lukasa]

I think at this point I have like some legally mandated requirement to inform you that my professional recommendation is that you set httplib on fire and never use it again.

You and I are in agreement here, which is why we're having this discussion. This discussion is a precursor to doing exactly that. In fact, let me tell you now that I have a branch of urllib3 that shims h11 into an interface that looks a lot like httplib that is currently passing about 300 urllib3 tests. The goal here is to get to a place where we can drop h11 in and remove httplib without too many people noticing, and that means trying to reduce the churn in the exceptions we emit.

Also does this mean you're going to want an enforce_content_length=False option in h11? Because that's going to be messy...

No. It's only set that way for backward compatibility purposes, and adding h11 would be the definition of a backward-incompatible change (there's no way we totally hide that change from our users), so we can change that behaviour too.

So AFAICT the actual current rule is that urllib3 will raise IncompleteRead on any error that occurs while reading the body, regardless of whether it's due to a connection drop and regardless of what kind of framing is being used, and it will attach some metadata that might or might not mean anything.

Yes, that's true, but the metadata that may be wrong almost always comes out of httplib. Now that we're assuming httplib's job, I'd like to do better than it. Getting this answer correct is entirely do-able: it is possible to distinguish malformed chunk bodies from truncated response bodies, and in the case of the latter to entirely explain how short it came out.

However, you may be right, a better approach may be to simply refuse to give that information and treat them all as "TruncatedResponse" and call it a day. I suspect we'll find that annoying from a debugging perspective: in particular, for HTTPS it becomes basically impossible to introspect what is wrong with the body without having pretty in-depth knowledge of the internals of h11 and urllib3 (you need to be able to see what code paths are being executed). For that reason if nothing else it would be nicer to provide some kind of useful information that is human-readable. We don't need it to be machine-readable, that's true, though in this case I tend to err on the side of making it available to machines once we're playing with this stuff.