-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
On server, include receiving HTTP/2 settings in handshake timeout #13336
Conversation
|
|
|
Noah, can you please take a look at this, since Craig is out? Unless you have a better suggestion, I'm thinking of changing the bad_client framework to use the handshaker code so that I can add a test for this functionality. Thanks! |
|
|
|
Let's talk tomorrow (or after break) about how best to test this. I think we are in need of a method or framework (maybe) to test chttp2 edge cases. I am running into that problem trying to test a fix for #13062. That PR will end up doing a similar thing to this, and I will build off this. |
|
|
|
I've added a test here that I think verifies that the change works as required. PTAL. Thanks! |
|
|
|
|
|
|
|
|
|
|
|
close_transport_locked(exec_ctx, t, close_transport); | ||
if (op->disconnect_with_error != GRPC_ERROR_NONE) { | ||
close_transport_locked(exec_ctx, t, op->disconnect_with_error); | ||
if (t->notify_on_receive_settings != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't this in close_transport_locked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that does look like a better place for it. Done.
|
|
|
|
|
1 similar comment
|
Apply #13336 to v1.8.x branch.
Apply #13336 to v1.7.x branch.
I think this is right, but I'd like confirmation both that (a) it should do the right thing and (b) it's the right way to implement this.
Also, I'd welcome suggestions on how to test this. My first thought was to use the bad_client framework, but it looks like bad_client directly creates a chttp2 transport rather than using the handshaking code from chttp2_server.cc, which is where the handshake timeout is enforced. Would it make sense to modify it to use the handshaking code? Or should I just write an independent test that does something similar? Alternative suggestions welcome.