-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a check to guarantee pingFuture == null
before the scheduling SGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.