Don't schedule multiple pings. #3295
Conversation
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure. |
| @@ -170,8 +171,10 @@ public synchronized void onDataReceived() { | |||
| } | |||
| // schedule a new ping | |||
| state = State.PING_SCHEDULED; | |||
| pingFuture = | |||
| scheduler.schedule(sendPing, keepAliveTimeInNanos, TimeUnit.NANOSECONDS); | |||
| if (pingFuture == null) { | |||
There was a problem hiding this comment.
You can move this condition check if (pingFuture == null) up by replacing the condition check if (state == State.PING_SCHEDULED) above with it.
Edited:
You can remove this condition check if (pingFuture == null) and replace the condition check if (state == State.PING_SCHEDULED) above with if (pingFuture != null).
There was a problem hiding this comment.
Hm, that seems reasonable, but AFAIU onDataReceived can happen while we are in State.IDLE/IDLE_WITH_PING_SENT and so with your suggestion we would move to PING_SCHEDULED when really we might want to stay in IDLE. (Or something like that.)
I did make your suggested change and the tests still pass though...
There was a problem hiding this comment.
I agree that we should leave this as-is. Mixing pingFuture checks with state machine checks makes the code complicated and easily broken (as is evidenced by the change breaking the IDLE case).
There was a problem hiding this comment.
@stephenh Your argument makes sense. We should stay IDLE when we are in IDLE while ping had been scheduled, and then onDataReceived. But the check if (pingFuture == null) still looks strange, may be Preconditions.checkState(pingFuture == null, "There should be no outstanding pingFuture") at this place.
There was a problem hiding this comment.
Adding a check to guarantee pingFuture == null before the scheduling SGTM.
There was a problem hiding this comment.
Ah sure, that's a good idea.
|
ok to test |
| @@ -69,6 +69,7 @@ public void run() { | |||
| // Schedule a shutdown. It fires if we don't receive the ping response within the timeout. | |||
| shutdownFuture = scheduler.schedule(shutdown, keepAliveTimeoutInNanos, | |||
| TimeUnit.NANOSECONDS); | |||
| pingFuture = null; | |||
There was a problem hiding this comment.
It seems like this should be done in all cases (but doesn't matter with DELAYED). If the state is IDLE, the current code would prevent all further PINGS (because it would never be rescheduled). Tying pingFuture == null to when there is no callback scheduled prevents having to consider most of those cases. So move clearing the field to before the if?
There was a problem hiding this comment.
Great catch. I updated the transportGoesIdle test to reproduce this "will never reschedule" bug, and then fixed it by moving the pingFuture = null to the top of the Runnable.
| @@ -170,8 +171,10 @@ public synchronized void onDataReceived() { | |||
| } | |||
| // schedule a new ping | |||
| state = State.PING_SCHEDULED; | |||
| pingFuture = | |||
| scheduler.schedule(sendPing, keepAliveTimeInNanos, TimeUnit.NANOSECONDS); | |||
| if (pingFuture == null) { | |||
There was a problem hiding this comment.
I agree that we should leave this as-is. Mixing pingFuture checks with state machine checks makes the code complicated and easily broken (as is evidenced by the change breaking the IDLE case).
If onTransportActive ran while SendPing was already scheduled, we would schedule another SendPing, which seems fine, but the server might observe us sending pings too quickly, and make us GOAWAY. Fixes grpc#3274.
|
Waiting for @dapengzhang0's review before merging. |
There was a problem hiding this comment.
LGTM. Mocking Future might produce some errorprone warnings internally, but as we may rewrite the tests per #2868 in the future, it fine for now.
|
@stephenh thanks a lot for the fix! |
If onTransportActive ran while SendPing was already scheduled, we would
schedule another SendPing, which seems fine, but the server might observe
us sending pings too quickly, and make us GOAWAY.
Fixes #3274.