Fix server-side keepalive shutdown process #10494
Conversation
|
| @@ -543,6 +543,10 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, | |||
| exec_ctx, &t->keepalive_ping_timer, | |||
| gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), t->keepalive_time), | |||
| &t->init_keepalive_ping_locked, gpr_now(GPR_CLOCK_MONOTONIC)); | |||
| } else { | |||
| /* Use GRPC_CHTTP2_KEEPALIVE_STATE_DYING to indicate there are no inflight | |||
There was a problem hiding this comment.
Is it worth adding a new state here - since this seems to be in initializing code path.. perhaps something like GRPC_CHTTP2_KEEPALIVE_STATE_NONE or GRPC_CHTTP2_KEEPALIVE_STATE_IDLE or something like that ?
There was a problem hiding this comment.
Thanks a lot for the review. Added GRPC_CHTTP2_KEEPALIVE_STATE_DISABLED to indicate the keepalive timers are not set.
There was a problem hiding this comment.
It might help if you add some description to this PR as to what was the issue..
I see that the main change here is to cancel the timers even on server (we previously only seem to be doing this on clients). I am not very clear about the design (i.e why was it only on clients previously?..and what changed recently that makes it okay to execute that code on both server and client) etc..
y-zeng
force-pushed
the
fix_server_keepalive
branch
from
ea1b39f to
aaa779e
Compare
|
Added more context in the description. It should be canceled on both the client and server. #9902 forgot to cancel it on server, but luckily the cancellation path of bandwidth estimator helped to cancel it, so it did not cause any trouble (also not caught by our tests 😵 ) . |
|
Thanks Yuchen. LGTM |
|
|
|
Known issues: |
#9468 added client-side keepalive timers, #9902 extended them to servers. But these timers were not canceled explicitly on servers when channels were closed by
close_transport_locked. They were canceled in the cancellation path of the bandwidth estimator.This PR cancels keepalive timers explicitly on servers, so that the cancellation of keepalive timers won't rely on the cancellation of the bandwidth estimator.