Skip to content

Honor max concurrent streams - #89

Merged
lovelydinosaur merged 8 commits into
masterfrom
honor-max-concurrent-streams
May 14, 2020
Merged

Honor max concurrent streams#89
lovelydinosaur merged 8 commits into
masterfrom
honor-max-concurrent-streams

Conversation

@lovelydinosaur

Copy link
Copy Markdown
Contributor

Closes#85

Work based off #38

@yeraydiazdiazyeraydiazdiaz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Left a couple of specific comments in the code.

Checking out the branch and testing against this very simple script:

importasyncioimporthttpxasyncdefmain(url, n):
asyncwithhttpx.AsyncClient(
timeout=httpx.Timeout(5.0),
http2=True,
) asclient:
awaitasyncio.gather(*[request(client, url, i) foriinrange(n)])
asyncdefrequest(client, url, i):
response=awaitclient.get(url)
assertresponse.status_code==200assertresponse.http_version=="HTTP/2"if__name__=="__main__":
asyncio.run(main("https://example.org", 200))

It fails around 1/2 to 1/3 of the time with the following traceback:

Traceback (most recent call last):
File ".dev/gather_http2_reuse.py", line 23, in <module>
asyncio.run(main("https://example.org", 200))
File "/Users/yeray/.pyenv/versions/3.8.2/lib/python3.8/asyncio/runners.py", line 43, in run
return loop.run_until_complete(main)
File "/Users/yeray/.pyenv/versions/3.8.2/lib/python3.8/asyncio/base_events.py", line 616, in run_until_complete
return future.result()
File ".dev/gather_http2_reuse.py", line 13, in main
await asyncio.gather(*[request(client, url, i) for i in range(n)])
File ".dev/gather_http2_reuse.py", line 17, in request
response = await client.get(url)
File "/Users/yeray/code/personal/_forks/httpx/httpx/_client.py", line 1275, in get
return await self.request(
File "/Users/yeray/code/personal/_forks/httpx/httpx/_client.py", line 1117, in request
response = await self.send(
File "/Users/yeray/code/personal/_forks/httpx/httpx/_client.py", line 1138, in send
response = await self.send_handling_redirects(
File "/Users/yeray/code/personal/_forks/httpx/httpx/_client.py", line 1165, in send_handling_redirects
response = await self.send_handling_auth(
File "/Users/yeray/code/personal/_forks/httpx/httpx/_client.py", line 1202, in send_handling_auth
response = await self.send_single_request(request, timeout)
File "/Users/yeray/code/personal/_forks/httpx/httpx/_client.py", line 1234, in send_single_request
) = await dispatcher.request(
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/connection_pool.py", line 152, in request
response = await connection.request(
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/connection.py", line 78, in request
return await self.connection.request(method, url, headers, stream, timeout)
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/http2.py", line 116, in request
return await h2_stream.request(method, url, headers, stream, timeout)
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/http2.py", line 286, in request
status_code, headers = await self.receive_response(timeout)
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/http2.py", line 338, in receive_response
event = await self.connection.wait_for_event(self.stream_id, timeout)
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/http2.py", line 192, in wait_for_event
await self.receive_events(timeout)
File "/Users/yeray/code/personal/_forks/httpcore/httpcore/_async/http2.py", line 206, in receive_events
raise ProtocolError(event)
httpcore._exceptions.ProtocolError: <StreamReset stream_id:363, error_code:ErrorCodes.REFUSED_STREAM, remote_reset:True>

Not really sure why it's only sometimes though, curious to know if you're also getting it.

Comment threadhttpcore/_backends/asyncio.py Outdated
Comment threadhttpcore/_async/http2.py Outdated
Comment threadhttpcore/_async/http2.py
Comment threadhttpcore/_async/http2.py Outdated
@lovelydinosaur

Copy link
Copy Markdown
ContributorAuthor

I’ve not seen that no - is that before or after this change, or both?

@yeraydiazdiaz

Copy link
Copy Markdown
Contributor

That happens on master as well, but usually fails with a "no more streams available" error which your change fixes.

@lovelydinosaur

Copy link
Copy Markdown
ContributorAuthor

I've addressed the error_code:ErrorCodes.REFUSED_STREAM issue now.
The semaphore release in _response_closed needs to occur after the network close.

@lovelydinosaur

Copy link
Copy Markdown
ContributorAuthor

Righty. I think we're there now.
Thanks so much for your review on this one - it was invaluable.

@yeraydiazdiaz

Copy link
Copy Markdown
Contributor

My pleasure! Awesome work 🚀

@lovelydinosaur
lovelydinosaur merged commit 717da48 into masterMay 14, 2020
@lovelydinosaur
lovelydinosaur deleted the honor-max-concurrent-streams branch May 14, 2020 14:24
This was referenced May 14, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Don't exceed MAX_CONCURRENT_STREAMS when sending HTTP/2 requests

2 participants

@lovelydinosaur@yeraydiazdiaz