diff --git a/quic/api/QuicSocket.h b/quic/api/QuicSocket.h index d348b69c6..0fd73cf2f 100644 --- a/quic/api/QuicSocket.h +++ b/quic/api/QuicSocket.h @@ -1124,24 +1124,17 @@ class QuicSocket { * Invoked if the ping times out */ virtual void pingTimeout() noexcept = 0; - - /** - * Invoked when a ping is received - */ - virtual void onPing() noexcept = 0; }; - /** - * Set the ping callback - */ - virtual folly::Expected setPingCallback( - PingCallback* cb) = 0; - /** * Send a ping to the peer. When the ping is acknowledged by the peer or * times out, the transport will invoke the callback. + * + * If 'callback' is nullptr, or pingTimeout is 0, no callback is scheduled. */ - virtual void sendPing(std::chrono::milliseconds pingTimeout) = 0; + virtual void sendPing( + PingCallback* callback, + std::chrono::milliseconds pingTimeout) = 0; /** * Get information on the state of the quic connection. Should only be used diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index 8180320d0..70e241c23 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -1400,16 +1400,7 @@ folly:: } } -void QuicTransportBase::handlePingCallbacks() { - if (conn_->pendingEvents.notifyPingReceived && pingCallback_ != nullptr) { - conn_->pendingEvents.notifyPingReceived = false; - runOnEvbAsync([](auto self) { - if (self->pingCallback_) { - self->pingCallback_->onPing(); - } - }); - } - +void QuicTransportBase::handlePingCallback() { if (!conn_->pendingEvents.cancelPingTimeout) { return; // nothing to cancel } @@ -1420,11 +1411,7 @@ void QuicTransportBase::handlePingCallbacks() { } pingTimeout_.cancelTimeout(); if (pingCallback_ != nullptr) { - runOnEvbAsync([](auto self) { - if (self->pingCallback_) { - self->pingCallback_->pingAcknowledged(); - } - }); + runOnEvbAsync([](auto self) { self->pingCallback_->pingAcknowledged(); }); } conn_->pendingEvents.cancelPingTimeout = false; } @@ -1717,7 +1704,7 @@ void QuicTransportBase::processCallbacksAfterNetworkData() { } conn_->pendingCallbacks.clear(); - handlePingCallbacks(); + handlePingCallback(); if (closeState_ != CloseState::OPEN) { return; } @@ -2446,19 +2433,9 @@ void QuicTransportBase::checkForClosedStream() { } } -folly::Expected QuicTransportBase::setPingCallback( - PingCallback* cb) { - if (closeState_ != CloseState::OPEN) { - return folly::makeUnexpected(LocalErrorCode::CONNECTION_CLOSED); - } - VLOG(4) << "Setting ping callback " - << " cb=" << cb << " " << *this; - - pingCallback_ = cb; - return folly::unit; -} - -void QuicTransportBase::sendPing(std::chrono::milliseconds pingTimeout) { +void QuicTransportBase::sendPing( + PingCallback* callback, + std::chrono::milliseconds pingTimeout) { /* Step 0: Connection should not be closed */ if (closeState_ == CloseState::CLOSED) { return; @@ -2469,8 +2446,8 @@ void QuicTransportBase::sendPing(std::chrono::milliseconds pingTimeout) { updateWriteLooper(true); // Step 2: Schedule the timeout on event base - if (pingCallback_ && pingTimeout != 0ms) { - schedulePingTimeout(pingCallback_, pingTimeout); + if (callback && pingTimeout != 0ms) { + schedulePingTimeout(callback, pingTimeout); } } @@ -2522,11 +2499,7 @@ void QuicTransportBase::pingTimeoutExpired() noexcept { if (pingCallback_ == nullptr) { return; } - runOnEvbAsync([](auto self) { - if (self->pingCallback_) { - self->pingCallback_->pingTimeout(); - } - }); + runOnEvbAsync([](auto self) { self->pingCallback_->pingTimeout(); }); } void QuicTransportBase::pathValidationTimeoutExpired() noexcept { diff --git a/quic/api/QuicTransportBase.h b/quic/api/QuicTransportBase.h index 702f140dd..15c56b670 100644 --- a/quic/api/QuicTransportBase.h +++ b/quic/api/QuicTransportBase.h @@ -208,10 +208,8 @@ class QuicTransportBase : public QuicSocket, QuicStreamPrioritiesObserver { StreamId id, QuicErrorCode error) override; - folly::Expected setPingCallback( - PingCallback* cb) override; - - void sendPing(std::chrono::milliseconds pingTimeout) override; + void sendPing(PingCallback* callback, std::chrono::milliseconds pingTimeout) + override; const QuicConnectionStateBase* getState() const override { return conn_.get(); @@ -704,7 +702,7 @@ class QuicTransportBase : public QuicSocket, QuicStreamPrioritiesObserver { void updateReadLooper(); void updatePeekLooper(); void updateWriteLooper(bool thisIteration); - void handlePingCallbacks(); + void handlePingCallback(); void handleKnobCallbacks(); void handleAckEventCallbacks(); void handleCancelByteEventCallbacks(); diff --git a/quic/api/test/MockQuicSocket.h b/quic/api/test/MockQuicSocket.h index 8ce95d975..09b984acb 100644 --- a/quic/api/test/MockQuicSocket.h +++ b/quic/api/test/MockQuicSocket.h @@ -244,10 +244,7 @@ class MockQuicSocket : public QuicSocket { MOCK_METHOD2( maybeResetStreamFromReadError, folly::Expected(StreamId, QuicErrorCode)); - MOCK_METHOD1( - setPingCallback, - folly::Expected(PingCallback*)); - MOCK_METHOD1(sendPing, void(std::chrono::milliseconds)); + MOCK_METHOD2(sendPing, void(PingCallback*, std::chrono::milliseconds)); MOCK_CONST_METHOD0(getState, const QuicConnectionStateBase*()); MOCK_METHOD0(isDetachable, bool()); MOCK_METHOD1(attachEventBase, void(folly::EventBase*)); diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index 35eee6cf0..0e666d4b3 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -131,7 +131,6 @@ class TestPingCallback : public QuicSocket::PingCallback { public: void pingAcknowledged() noexcept override {} void pingTimeout() noexcept override {} - void onPing() noexcept override {} }; class TestByteEventCallback : public QuicSocket::ByteEventCallback { @@ -311,16 +310,18 @@ class TestQuicTransport ackTimeout_.timeoutExpired(); } - void invokeSendPing(std::chrono::milliseconds interval) { - sendPing(interval); + void invokeSendPing( + quic::QuicSocket::PingCallback* cb, + std::chrono::milliseconds interval) { + sendPing(cb, interval); } void invokeCancelPingTimeout() { pingTimeout_.cancelTimeout(); } - void invokeHandlePingCallbacks() { - handlePingCallbacks(); + void invokeHandlePingCallback() { + handlePingCallback(); } void invokeHandleKnobCallbacks() { @@ -3456,12 +3457,11 @@ TEST_F(QuicTransportImplTest, SuccessfulPing) { auto conn = transport->transportConn; std::chrono::milliseconds interval(10); TestPingCallback pingCallback; - transport->setPingCallback(&pingCallback); - transport->invokeSendPing(interval); + transport->invokeSendPing(&pingCallback, interval); EXPECT_EQ(transport->isPingTimeoutScheduled(), true); EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); conn->pendingEvents.cancelPingTimeout = true; - transport->invokeHandlePingCallbacks(); + transport->invokeHandlePingCallback(); evb->loopOnce(); EXPECT_EQ(transport->isPingTimeoutScheduled(), false); EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); @@ -3471,13 +3471,12 @@ TEST_F(QuicTransportImplTest, FailedPing) { auto conn = transport->transportConn; std::chrono::milliseconds interval(10); TestPingCallback pingCallback; - transport->setPingCallback(&pingCallback); - transport->invokeSendPing(interval); + transport->invokeSendPing(&pingCallback, interval); EXPECT_EQ(transport->isPingTimeoutScheduled(), true); EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); conn->pendingEvents.cancelPingTimeout = true; transport->invokeCancelPingTimeout(); - transport->invokeHandlePingCallbacks(); + transport->invokeHandlePingCallback(); EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); } diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index f4dfe5433..87877cb14 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -566,7 +566,6 @@ void QuicClientTransport::processPacketData( // Ping isn't retransmittable. But we would like to ack them early. // So, make Ping frames count towards ack policy pktHasRetransmittableData = true; - conn_->pendingEvents.notifyPingReceived = true; break; case QuicFrame::Type::PaddingFrame: break; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 6a7ff1926..cb990b7f8 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -1149,7 +1149,6 @@ void onServerReadDataFromOpen( // Ping isn't retransmittable data. But we would like to ack them // early. pktHasRetransmittableData = true; - conn.pendingEvents.notifyPingReceived = true; break; case QuicFrame::Type::PaddingFrame: break; diff --git a/quic/state/StateData.h b/quic/state/StateData.h index e4f8d85d4..5998fd76f 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -560,8 +560,6 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { bool cancelPingTimeout{false}; - bool notifyPingReceived{false}; - // close transport when the next packet number reaches kMaxPacketNum bool closeTransport{false};