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

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

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
uvicorn/main.py Outdated Show resolved Hide resolved
Copy link
Sponsor 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.

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.

docs/settings.md Outdated Show resolved Hide resolved
@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
Sponsor 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
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@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.

Copy link
Sponsor 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
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
Sponsor 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.

tests/protocols/test_http.py Show resolved Hide resolved


@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 added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
…ncode#1514)

* Solves encode#1234

* Update docs/settings.md

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

* Update uvicorn/main.py

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

* Fix lint

* Fixing tests

* Tweak tests a bit

* Remove event_loop fixture

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Kludex added a commit that referenced this pull request Oct 29, 2022
…1514)

* Solves #1234

* Update docs/settings.md

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

* Update uvicorn/main.py

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

* Fix lint

* Fixing tests

* Tweak tests a bit

* Remove event_loop fixture

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting author Waiting for author's reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants