okhttp: forceful close after MAX_CONNECTION_AGE_GRACE_TIME #9968
Conversation
| @@ -926,7 +935,7 @@ public void ping(boolean ack, int payload1, int payload2) { | |||
| return; | |||
| } | |||
| if (GRACEFUL_SHUTDOWN_PING == payload) { | |||
| triggerGracefulSecondGoaway(); | |||
| triggerGracefulSecondGoaway(null); | |||
There was a problem hiding this comment.
This is actually the primary flow. secondGoawayTimer is just in case the ping takes too long, we still want to continue the shutdown process. So using null here means that shutdown(void) is correct, but the max connection age won't have a grace time.
There was a problem hiding this comment.
Ah right that's true.
So after the first goaway and ping, the secondGoawayTimer is set with 1s ping timeout.
A normal successful ping ack within 1s cancels the timer and trigger second goAway and graceful shutdown. No ping after 1s also triggers the same path.
The ping method does not take any more parameter, as it implements frame handler interface. So I made the grace time a field. And it is synchronized on the lock to allow read and write to be thread safe.
YifeiZhuang
force-pushed
the
okhttp-grace-shutdown
branch
from
f653807
to
e3c5690
Compare
…p-grace-shutdown
| @@ -289,6 +294,10 @@ private void triggerGracefulSecondGoaway() { | |||
| } else { | |||
| frameWriter.flush(); | |||
| } | |||
| if (gracefulShutdownPeriod != null) { | |||
| forcefulCloseTimer = scheduledExecutorService.schedule( | |||
| this::triggerForcefulClose, gracefulShutdownPeriod, TimeUnit.NANOSECONDS); | |||
There was a problem hiding this comment.
This changes the behavior when someone is doing:
server.shutdown();
server.awaitTerminated(10000, TimeUnit.SECONDS);There was a problem hiding this comment.
I forgot to mention that #9649 changed the behavior of this. So I put it back.
So sound like we should do graceful shutdown when server.shutdown?
There was a problem hiding this comment.
I don't see any change to triggerGracefulSecondGoaway() in #9649. We will do "double GOAWAY" graceful shutdown when server.shutdown(). That's what the code is doing already. server.shutdown() doesn't have a forceful close though.
There was a problem hiding this comment.
never mind about #9649.
I updated server.shutdown() to avoid forceful close.
fixes #9721