hyper icon indicating copy to clipboard operation
hyper copied to clipboard

Come up with an API for 1XX responses.

Open Lukasa opened this issue 10 years ago • 21 comments

This is not something httplib supports, so we have nowhere to take our lead from. This means we can do this right (or at least right-ish).

So what's our idealised API? /cc @sigmavirus24 for his brains, @shazow as the likely primary consumer of the API.

Lukasa avatar Aug 07 '14 12:08 Lukasa

I don't know the draft well enough. Let me go read that and then I'll come back with ideas.

sigmavirus24 avatar Aug 07 '14 13:08 sigmavirus24

Yea same... I'd love to see some potential code examples of what such an API might look like, and give feedback on that. :)

shazow avatar Aug 07 '14 20:08 shazow

I don't see any references to 100-continue in the draft linked in #68. Will it work the same way as it does in HTTP/1.1 where it is an interim (non-authoritative) response? If that's the case, I'm not sure we need a special API but I also don't think we should swallow/ignore it like httplib. I'll also work on the assumption that in HTTP/2 the client doesn't actually have to wait to receive the 100 Continue status ([like it's described in HTTP/1.1).

I wonder if an improvement would involve stealing urllib3's Timeout class so the user could specify the amount of time they want to wait for the server to respond with 100 Continue after which hyper then just proceeds to send the request. (Obviously if the server responds with 100 Continue before the timeout triggers, then we wouldn't wait the entire timeout.) At the same time, I'm not sure if there's a good way to integrate this with the current API or if there's a better way to implement this without inspecting headers set by the user.

The advantage to this way of doing this way is that the API is kept simple and flexible. Meanwhile, I'm unsure how to represent to the user that it received a 100 Continue. At the same time, if we handle everything correctly I'm not sure the user needs to know.

sigmavirus24 avatar Aug 07 '14 20:08 sigmavirus24

Yeah, 1XX is not special in HTTP/2, it behaves exactly as it does in HTTP/1.1. This is therefore an opportunity for us to work out how we'd like provisional responses to work.

First, note that 100 is not the only provisional response. The 100 Continue flow is an interesting one, but we may well want to expose the possibility of acting on other responses.

One API option is to have the user handle the 100 Continue flow themselves. That would lead to an API a bit like this:

# Build the request but don't send any body data.
c = HTTP20Connection('google.com:443')
c.putrequest('POST', '/some_endpoint')
c.putheader('Content-Type', 'application/octet-stream')
c.putheader('Content-Length', len(big_data))
c.endheaders()

# Wait for a provisional response for 5 seconds.
resp = c.getresponse(timeout=5)

if resp is None or resp.status == 100:
    c.send(big_data)
else:
    # Handle final status code, probably an error.

This method has the advantage of allowing for other 1XX status codes, but forces the user to manage the entire flow. I don't know that that's a problem for me. We'd need to rework HTTP20Connection.request to work better here, or maybe have it handle the 1XX flow itself. Not sure. Thoughts?

Lukasa avatar Aug 08 '14 12:08 Lukasa

Not to be a pain, but I assume you also meant to include c.putheader('Expect', '100-continue').

I think this design would work fine for anyone using hyper directly. I guess urllib3 would do some header inspection at this point? (e.g., if headers.get('Expect', '') == '100-continue': # Wait for 100 response.)

sigmavirus24 avatar Aug 08 '14 13:08 sigmavirus24

I did, yeah. =P

I think that's correct. Hyper's job is really to make it possible to handle provisional responses, whereas urllib3's job may be to abstract them away. Does that seem reasonable, @shazow?

(A side note: we can also handle trailers, which are headers that come after a chunked body has been received. Right now they're just transparently merged with the original headers, but it might be better to provide an alternate way to receive them. Any preferences?)

Lukasa avatar Aug 11 '14 10:08 Lukasa

Yea sounds like a decent low-level interface, we can always add more abstractions on top of it.

shazow avatar Aug 11 '14 17:08 shazow

(A side note: we can also handle trailers, which are headers that come after a chunked body has been received. Right now they're just transparently merged with the original headers, but it might be better to provide an alternate way to receive them. Any preferences?)

Maybe combined headers live in .headers, and then you have .headers_pre and .headers_post or something?

shazow avatar Aug 11 '14 17:08 shazow

Maybe combined headers live in .headers, and then you have .headers_pre and .headers_post or something?

I was thinking along the same lines, but was thinking of only having .headers and .trailers where the difference would be the equivalent to .headers_pre. Seem reasonable?

sigmavirus24 avatar Aug 11 '14 18:08 sigmavirus24

So they would not be combined? I guess that can work. On Aug 11, 2014 11:30 AM, "Ian Cordasco" [email protected] wrote:

Maybe combined headers live in .headers, and then you have .headers_pre and .headers_post or something?

I was thinking along the same lines, but was thinking of only having .headers and .trailers where the difference would be the equivalent to .headers_pre. Seem reasonable?

— Reply to this email directly or view it on GitHub https://github.com/Lukasa/hyper/issues/71#issuecomment-51820727.

shazow avatar Aug 11 '14 19:08 shazow

Sorry that wasn't clear. .headers would be as you described ("regular" headers + trailers). .trailers would be the trailers that Hyper received. To get the "regular" headers, you would exclude anything in .trailers from what you find in .headers. I would expect it would not be too terribly difficult to figure that out but y'all should correct me if I'm wrong.

sigmavirus24 avatar Aug 11 '14 19:08 sigmavirus24

How does excluding work if there are duplicates? I'd keep them separate. Maybe have a combined one in addition, either way. On Aug 11, 2014 12:52 PM, "Ian Cordasco" [email protected] wrote:

Sorry that wasn't clear. .headers would be as you described ("regular" headers + trailers). .trailers would be the trailers that Hyper received. To get the "regular" headers, you would exclude anything in .trailers from what you find in .headers. I would expect it would not be too terribly difficult to figure that out but y'all should correct me if I'm wrong.

— Reply to this email directly or view it on GitHub https://github.com/Lukasa/hyper/issues/71#issuecomment-51831271.

shazow avatar Aug 11 '14 19:08 shazow

I think the trailers and headers are logically a single header block. This means you should only be able to duplicate headers that can be represented as comma-separated lists (and the Set-Cookie header).

With that said, I'd be leaning towards having headers and trailers separate, and then have a simple property that combines them.

Lukasa avatar Aug 11 '14 20:08 Lukasa

There will always be that one edge case where having the original format is meaningful. Be careful mangling the returned data too much, lest your code turns into httplib. :p On Aug 11, 2014 1:50 PM, "Cory Benfield" [email protected] wrote:

I think the trailers and headers are logically a single header block. This means you should only be able to duplicate headers that can be represented as comma-separated lists (and the Set-Cookie header).

With that said, I'd be leaning towards having headers and trailers separate, and then have a simple property that combines them.

— Reply to this email directly or view it on GitHub https://github.com/Lukasa/hyper/issues/71#issuecomment-51838725.

shazow avatar Aug 11 '14 21:08 shazow

So .headers, .trailers, and .all_headers (or .combined_headers)?

sigmavirus24 avatar Aug 12 '14 14:08 sigmavirus24

Maybe.

My issue there is that for non-expert users .headers is an attractive nuisance that may be missing useful information available in .all_headers. I'm not sure of a good way to handle this API without being really awkward.

For that matter, it seems like trailers are treated conceptually as part of the headers for a request. That's weird: they aren't headers in any sense.

Here's another question: if you request the equivalent of .all_headers before you've read any of the body, in HTTP/2 we'll need to consume the entire request body before we know for sure that there aren't any to come. That makes this a really magnificent footgun.

This entire discussion makes me want to back away from ever showing the combined set, and to simply have .headers and .trailers and then provide utility functions for combining the two.

Lukasa avatar Aug 12 '14 14:08 Lukasa

I think that API seems best.

sigmavirus24 avatar Aug 12 '14 15:08 sigmavirus24

+1, have higher level libs merge them.

On Tue, Aug 12, 2014 at 8:30 AM, Ian Cordasco [email protected] wrote:

I think that API seems best.

— Reply to this email directly or view it on GitHub https://github.com/Lukasa/hyper/issues/71#issuecomment-51930920.

shazow avatar Aug 12 '14 17:08 shazow

Further thought on this makes me wonder whether the Expect: 100 Continue flow should actually be special-cased by hyper. Something like:

c = HTTPConnection('somefileuploadsite.com')
c.request('POST', '/myfile', body=massive_file, expect_continue=True)
r = c.get_response()
assert r.status_code == 200

Under the covers, hyper did the 'wait for 100 response' logic and sent the body onwards.

The advantage of this API is that the user doesn't need to worry about the exact semantics of the 100 response. All other provisional responses will be returned as normal, and the user will (as normal) be allowed/expected to retrieve the socket object if they need it.

Lukasa avatar Mar 02 '15 09:03 Lukasa

So, here's the fun thing. The spec (last I checked) said to wait for a 100 Continue or a reasonable amount of time. What if expect_continue allowed for a float that was essentially "After x seconds, send the file anyway"

sigmavirus24 avatar Mar 23 '15 21:03 sigmavirus24

Ooh, that's a good idea. It'll also allow for true which will choose a reasonable time (something like 1 second), but can take the float.

Lukasa avatar Mar 23 '15 21:03 Lukasa