python-hyper / hyper

HTTP/2 for Python.
http://hyper.rtfd.org/en/latest/
MIT License
1.05k stars 191 forks source link

Streams code snippet needs updating #352

Closed paul-hammant closed 6 years ago

paul-hammant commented 6 years ago

Ref http://hyper.readthedocs.io/en/latest/quickstart.html#streams

second_response = c.get_response(second)

Causes TypeError: get_response() takes 1 positional argument but 2 were given when running. Taking out the second from the get_response method invocation means the error goes away. However, the second get_response times out.

Lukasa commented 6 years ago

That error occurs if HTTP/2 is not successfully negotiated.

paul-hammant commented 6 years ago

I'm using HTTP11Connection directly - are you 100% sure that HTTP20Connection is involved?

c = HTTP11Connection('192.168.1.178', 80)
hdrs = headers = {
    "Host": "192.168.1.178",
    "Authorization": "Basic cwF1brpqYXFhYeE=",
    "Referer": "http://192.168.1.178/myFolder/",
}

first = c.request('MKCOL', 'http://192.168.1.178/myFolder/1', body="", headers=hdrs)
second = c.request('MKCOL', 'http://192.168.1.178/myFolder/1/2', body="", headers=hdrs)

print("2: " + str(c.get_response(second).status))
print("1: " + str(c.get_response(first).status))
Lukasa commented 6 years ago

In that case you aren’t following the documentation. The documentation you linked to covers using either the generic interface or the HTTP/2 one: the HTTP/1.1 interface is different, and does not accept a stream ID parameter.

paul-hammant commented 6 years ago

That snippet I've supplied above works on a Apache2/Subversion server. I could pipeline (that's what you mean by 'stream', right?) 20 MKCOLs together and they're all action perfectly on the subversion backend. The line the fails? The c.get_response(..) fails as described.

I found your snippet of code and Hyper generally because I was searching for how to do this in Python given the Requests library does not support pipelining - https://en.wikipedia.org/wiki/HTTP_pipelining

Lukasa commented 6 years ago

Given that getting the responses fails I think we can safely say that the code does not work unless you have no interest in reading your responses. However, you have not understood the situation, so let me explain more fully.

get_response, in situations where HTTP/2 is in use, accepts a single argument: an integer called the “stream ID”. This integer is used to correlate which request you want to get the response for, and is a code part of the HTTP/2 protocol. HTTP/1.1 is not a multiplexed protocol: that is, you can only get responses in the order that you sent the requests. For this reason providing a stream ID makes no sense: HTTP/1.1 does not have streams, and you cannot ask for the response for request 5 without consuming that for requests 1 through 4. For this reason when using HTTP/1.1 the get_response method does not take an argument: it just returns the next response to get.

Remove the argument, get your responses in order, and it’ll be fine.

paul-hammant commented 6 years ago

It times out on the first get_response. Would you like me to make a full reproduction of that (admittedly modified description of the problem) for you with a suitable docker container?

Incidentally, there is an exception if I leave out body="" for MKCOL operations?

Lukasa commented 6 years ago

It would probably be better if you could show me what tcpdump thinks the response looks like. It’s possible that it is not structured in a way hyper understands right now.