binder: Let the server know when the client fails to authorize it.#12445
binder: Let the server know when the client fails to authorize it.#12445jdcormie merged 1 commit intogrpc:masterfrom
Conversation
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.
ejona86
left a comment
There was a problem hiding this comment.
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.
|
Thanks for looking! I believe your theory about the intent of the delayed setOutgoingBinder() call. After this PR, we'll depend on the Good point about those two early error cases. I updated the text of the bug. |
…rpc#12445) 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 grpc#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. Fixes grpc#12438
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