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.

Sign up for GitHub

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

Jump to bottom

Adjust max streams when remote settings change #652

Merged
merged 5 commits into from
Mar 16, 2023
Merged

Adjust max streams when remote settings change #652

merged 5 commits into from
Mar 16, 2023

Conversation

dunkmann00
Copy link
Contributor

If the remote server sends a settings update for max_concurrent_streams, check and potentially update the max number of streams we will use for a connection.

This is related to encode/httpx#2416.

@dunkmann00
Copy link
Contributor Author

Not sure where to put this so I'll just add it as another message. When I tested this against APNS, I was able to see the settings change come in. This is what the change in max concurrent streams specifically looked like:

First event:
ChangedSetting(setting=SettingCodes.MAX_CONCURRENT_STREAMS, original_value=None, new_value=1)

Second Event:
ChangedSetting(setting=SettingCodes.MAX_CONCURRENT_STREAMS, original_value=1, new_value=1000)

@dunkmann00
Adjust max streams when remote settings change
8d70914 
If the remote server sends a settings update for
`max_concurrent_streams`, check and potentially update the max number
of streams we will use for a connection.
@dunkmann00
Copy link
Contributor Author

Hello @tomchristie, just wanted to reach out to see if you or somebody from the encode team could review this? Thanks!

@tomchristie
Copy link
Member

Great stuff, thanks! 🎉

The CI reports a linting failure indicating that the async and sync versions don't match. You can resolve this locally by running scripts/unasync.

I'd suggest resolving that first.

Then you might want to take a look at https://github.com/encode/httpcore/blob/master/tests/_async/test_http2.py and see if you're able to add some test coverage for this.

@dunkmann00
Add aiter_stream sub to iter_stream
851c303
@dunkmann00
Add test for remote max streams update
bf604e0 
Improve testing for remote max streams update

This new test checks whether a change in the max streams setting is
properly handled. The max streams value is changed twice throughout the
mock stream. The first update increases the value while the second
update decreases it.
@dunkmann00
Copy link
Contributor Author

Okay I think this is good to go! Let me know if there are any other changes you would like me to make.

@dunkmann00
Copy link
Contributor Author

Hey @tomchristie! Just checking in to see what you think of the recent commits with the added tests?

@tomchristie
Copy link
Member

Great work, thanks!

@tomchristie tomchristie merged commit a174e66 into encode:master Mar 16, 2023
@tomchristie tomchristie mentioned this pull request Mar 16, 2023
Merged
2 tasks
@tomchristie
Copy link
Member

Would be interesting to see this working together with our latest logging update...

https://www.encode.io/httpcore/logging/

@dunkmann00
Copy link
Contributor Author

Thanks for the merge!

Would be interesting to see this working together with our latest logging update...

Are you planning on bumping the httpcore dep in httpx. I'm not in a hurry or anything, but it would be easier to try this out if it was in httpx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomchristie tomchristie tomchristie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dunkmann00 @tomchristie