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

Max concurrent stream improvements #90

Merged
merged 1 commit into from
May 14, 2020

Conversation

tomchristie
Copy link
Member

Right, there was some stuff I didn't quite get correct in #89

Absurdly I'd forgotten to use http2=True in my testings. This is the script I'm using now for this...

import asyncio
import httpcore


async def main():
    async with httpcore.AsyncConnectionPool(http2=True) as client:
        await asyncio.gather(*[request(client, idx) for idx in range(1000)])

async def request(client, idx):
    http_version, status_code, reason_phrase, headers, stream = await client.request(
        method=b'GET',
        url=(b'https', b'example.org', 443, b'/'),
        headers=[(b'host', b'example.org:443')]
    )

    try:
        body = b''.join([chunk async for chunk in stream])
    finally:
        await stream.aclose()


asyncio.run(main())

The changes here are:

  • We need to use the local settings to determine how many concurrent streams we can issue, not the remote settings.
  • The stream_id = self.h2_state.get_next_available_stream_id() needs to be part of the semaphore-limited wrapped code, or else it stops incrementing once we've hit the concurrency limit. As a result I've moved the wrapping layer out from AsyncHTTP2Stream onto AsyncHTTP2Connection instead.

Copy link
Contributor

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@tomchristie tomchristie merged commit 30847a0 into master May 14, 2020
@tomchristie tomchristie deleted the max-concurrent-stream-improvements branch May 14, 2020 15:44
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.

2 participants