Skip to content

Conversation

@jdcormie
Copy link
Member

@jdcormie jdcormie commented Oct 24, 2025

Avoids waiting for the handshake timeout on the server side in this case.

I also add test coverage for the !setOutgoingBinder() case to make sure it works in the new location.

My ulterior motive for this change is simplifying the client handshake code in preparation for #12398 -- An (impossible) !isShutdown() clause goes away for easy to understand reasons and I'll no longer have to pass the server's binder as an arg from async function to function in two separate handshake impls.

TGP passes: fusion2/OCL:823722865:BASE:823829545:1761379767122:2d595b69

Fixes #12438

Avoids handshake timeout on the server side in this case. Also has the
nice side effect that the client handshake code doesn't have to pass the
server's binder from async function to function as an argument.
@jdcormie jdcormie requested a review from ejona86 October 27, 2025 18:55
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I expect the code was delaying setOutgoingBinder() on purpose, to prevent accidentally sending on a unauthenticated binder. But that can be changed. It seems to have simply been a bug where the client wouldn't notify the server of failures during handshaking. I agree the code should notify the server when shut down while handshaking.

Note that the text of #12438 would seem to imply "Wire format version mismatch" and "Malformed SETUP_TRANSPORT data" aren't handled properly, and you didn't actually change the behavior here. But they don't respond because the handshake was too broken to figure out how to respond.

@jdcormie
Copy link
Member Author

Thanks for looking!

I believe your theory about the intent of the delayed setOutgoingBinder() call. After this PR, we'll depend on the !inState(READY) to prevent creating streams too early. As for accidental transactions during the handshake, I agree this deferral never accomplished much. The handshake code will always have to be carefully checked for security problems with or without it.

Good point about those two early error cases. I updated the text of the bug.

@jdcormie jdcormie merged commit 599a0a1 into grpc:master Oct 27, 2025
15 of 17 checks passed
@jdcormie jdcormie deleted the set-outgoing-binder-earlier branch October 27, 2025 20:52
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.

binder: client doesn't tell the server about errors during the handshake

2 participants