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

Allow configuring max_incomplete_event_size for h11 implementation #1514

Merged
merged 8 commits into from
Jun 22, 2022
Merged

Allow configuring max_incomplete_event_size for h11 implementation #1514

merged 8 commits into from
Jun 22, 2022

Conversation

gauravojha
Copy link
Contributor

@gauravojha gauravojha commented Jun 10, 2022 •
edited
Loading

PR to help solve #1234

@gauravojha gauravojha changed the title Solves #1234 Allow configuring max_incomplete_event_size for h11 implementation Jun 10, 2022
@Kludex Kludex added this to the Version 0.18.0 milestone Jun 11, 2022
Kludex
Kludex reviewed Jun 11, 2022
View reviewed changes
Kludex
Kludex reviewed Jun 12, 2022
View reviewed changes
Copy link
Member

@Kludex Kludex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've studied the issue again. I have questions:

  • Instead of accepting this PR, can we just work with httptools when there's a need for larger headers?
  • Can we improve the tests that have conditionals in the body of the test? 🤔

If using httptools is an alternative, then we can also document this.

@Kludex Kludex added the waiting author Waiting for author's reply label Jun 12, 2022
@euri10
Copy link
Member

euri10 commented Jun 14, 2022

  • Instead of accepting this PR, can we just work with httptools when there's a need for larger headers?

I think it's fair to have this flag for people not able to run uvicorn with C extensions

@Kludex
Copy link
Member

Kludex commented Jun 14, 2022

I think it's fair to have this flag for people not able to run uvicorn with C extensions

Perfect. 👍

@gauravojha
Copy link
Contributor Author

thanks @Kludex @euri10

Sorry for the delayed response. Let me take a stab at fixing the conditionals in the test

gauravojha and others added 4 commits June 14, 2022 09:55
@gauravojha
Copy link
Contributor Author

@Kludex let me know if these changes are closer to what you had in mind with the conditionals change in tests.

Kludex
Kludex approved these changes Jun 17, 2022
View reviewed changes
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

As reference: https://h11.readthedocs.io/en/latest/api.html?highlight=max-incomplete-event-size#the-connection-object

After seeing the new tests, I think we either should remove the parametrize, or go back to the previous: https://github.com/encode/uvicorn/pull/1514/files/bf74201a9887f2b2de5d5948f8f4a0a4aaf46fd3... I'll let @euri10 decide what he likes more.

@Kludex Kludex mentioned this pull request Jun 18, 2022
Merged
5 tasks
@Kludex Kludex requested a review from euri10 June 18, 2022 16:55
@euri10
Copy link
Member

euri10 commented Jun 20, 2022

After seeing the new tests, I think we either should remove the parametrize, or go back to the previous: https://github.com/encode/uvicorn/pull/1514/files/bf74201a9887f2b2de5d5948f8f4a0a4aaf46fd3... I'll let @euri10 decide what he likes more.

it feels ok to me this way

@Kludex
Copy link
Member

Kludex commented Jun 20, 2022

I've tweaked the tests a bit to be more clear... Can you check what you think @euri10 ? If you're fine, please merge it.

euri10
euri10 reviewed Jun 21, 2022
View reviewed changes
tests/protocols/test_http.py


@pytest.mark.anyio
@pytest.mark.skipif(HttpToolsProtocol is None, reason="httptools is not installed")
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@euri10 euri10 merged commit 81d647b into encode:master Jun 22, 2022
@gauravojha gauravojha deleted the event_size branch June 22, 2022 21:20
@Kludex Kludex mentioned this pull request Oct 1, 2022
Closed
2 tasks
@sacca97 sacca97 mentioned this pull request Oct 8, 2022
Closed
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
@gauravojha @Kludex
Allow configuring max_incomplete_event_size for h11 implementation (e…
a1e0576 
…ncode#1514)

* Solves encode#1234

* Update docs/settings.md

Co-authored-by: Marcelo Trylesinski <[email protected]>

* Update uvicorn/main.py

Co-authored-by: Marcelo Trylesinski <[email protected]>

* Fix lint

* Fixing tests

* Tweak tests a bit

* Remove event_loop fixture

Co-authored-by: Marcelo Trylesinski <[email protected]>
Kludex added a commit that referenced this pull request Oct 29, 2022
@gauravojha @Kludex
Allow configuring max_incomplete_event_size for h11 implementation (#…
6d3c84c 
…1514)

* Solves #1234

* Update docs/settings.md

Co-authored-by: Marcelo Trylesinski <[email protected]>

* Update uvicorn/main.py

Co-authored-by: Marcelo Trylesinski <[email protected]>

* Fix lint

* Fixing tests

* Tweak tests a bit

* Remove event_loop fixture

Co-authored-by: Marcelo Trylesinski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@euri10 euri10 euri10 approved these changes

@Kludex Kludex Kludex approved these changes

Assignees
No one assigned
Labels
waiting author Waiting for author's reply
Projects
None yet
Milestone
Version 0.18.0
Development

Successfully merging this pull request may close these issues.
3 participants
@gauravojha @euri10 @Kludex