Skip to content

Fix test_max_incomplete_event_size_countermeasure test#184

Open
anton-ryzhov wants to merge 1 commit intopython-hyper:masterfrom
anton-ryzhov:fix-test
Open

Fix test_max_incomplete_event_size_countermeasure test#184
anton-ryzhov wants to merge 1 commit intopython-hyper:masterfrom
anton-ryzhov:fix-test

Conversation

@anton-ryzhov
Copy link

This test was not actually testing what it should — max_incomplete_event_size limit is only checked for incomplete events, for request before "\r\n\r\n".

get_all_events() before "\r\n\r\n" conditionally raises an exception depending on max_incomplete_event_size

@anton-ryzhov anton-ryzhov marked this pull request as ready for review July 5, 2025 22:10
@anton-ryzhov
Copy link
Author

@njsmith does this PR miss something or could it be merged?

@njsmith
Copy link
Member

njsmith commented Feb 18, 2026

I guess I'm not seeing anything wrong with this, but I also don't understand what the purpose of this is? What was it not testing before that it does test now?

@anton-ryzhov
Copy link
Author

These two blocks demonstrate and test that the same payload is handled fine with max_incomplete_event_size=5000 but raises an error with max_incomplete_event_size=4000.

But in fact the first block doesn't ensure that, it doesn't fail if we set max_incomplete_event_size=1000. But it does fail if we add an assert.

So, the successful case was always successful regardless of the configuration

@njsmith
Copy link
Member

njsmith commented Feb 19, 2026

Ohh I see yeah that makes sense for the first change, good catch. Still don't think I understand the motivation for the second change?

@anton-ryzhov
Copy link
Author

Still don't think I understand the motivation for the second change?

To be honest I don't remember.

  • Maybe it was for sake of consistency, to check the same in both (line 428) cases
  • Maybe a semantic change, instead of "the very first event raises an error" to "at any step of processing, this error will be triggered"
  • Maybe it played some role in this change

Do you want me to revert it?

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

Comments