From 499b5fb0cebf893da1cc9707fa2bbe8eaf3e6c4e Mon Sep 17 00:00:00 2001 From: Junqi Wang Date: Wed, 1 Jul 2020 19:33:46 -0700 Subject: [PATCH] Remove continueOnNetworkUnreachable Reviewed By: yangchi Differential Revision: D22333283 fbshipit-source-id: b35fb9e6ba31e88faf2c92805edc2738cfde93f1 --- quic/api/IoBufQuicBatch.cpp | 19 +---- .../client/test/QuicClientTransportTest.cpp | 76 +------------------ quic/state/StateData.h | 3 - quic/state/TransportSettings.h | 12 --- 4 files changed, 3 insertions(+), 107 deletions(-) diff --git a/quic/api/IoBufQuicBatch.cpp b/quic/api/IoBufQuicBatch.cpp index 1b6e859c8..515ed8deb 100644 --- a/quic/api/IoBufQuicBatch.cpp +++ b/quic/api/IoBufQuicBatch.cpp @@ -66,20 +66,8 @@ void IOBufQuicBatch::reset() { } bool IOBufQuicBatch::isRetriableError(int err) { - if (err == EAGAIN || err == EWOULDBLOCK || err == ENOBUFS || - err == EMSGSIZE) { - return true; - } - auto now = Clock::now(); - if (conn_.transportSettings.continueOnNetworkUnreachable && - isNetworkUnreachable(err)) { - if (!conn_.continueOnNetworkUnreachableDeadline) { - conn_.continueOnNetworkUnreachableDeadline = - now + conn_.transportSettings.continueOnNetworkUnreachableDuration; - } - return now <= *conn_.continueOnNetworkUnreachableDeadline; - } - return false; + return err == EAGAIN || err == EWOULDBLOCK || err == ENOBUFS || + err == EMSGSIZE; } bool IOBufQuicBatch::flushInternal() { @@ -164,9 +152,6 @@ bool IOBufQuicBatch::flushInternal() { return false; // done } - // Reset the deadline after successful write - conn_.continueOnNetworkUnreachableDeadline = folly::none; - return true; // success, not done yet } } // namespace quic diff --git a/quic/fizz/client/test/QuicClientTransportTest.cpp b/quic/fizz/client/test/QuicClientTransportTest.cpp index 3bf347f10..a93e7fd6f 100644 --- a/quic/fizz/client/test/QuicClientTransportTest.cpp +++ b/quic/fizz/client/test/QuicClientTransportTest.cpp @@ -1927,79 +1927,6 @@ TEST_F(QuicClientTransportTest, NetworkUnreachableIsFatalToConn) { loopForWrites(); } -TEST_F(QuicClientTransportTest, NetworkUnreachableIsNotFatalIfContinue) { - TransportSettings settings; - settings.continueOnNetworkUnreachable = true; - client->setTransportSettings(settings); - client->addNewPeerAddress(serverAddr); - EXPECT_CALL(clientConnCallback, onConnectionError(_)).Times(0); - setupCryptoLayer(); - EXPECT_CALL(*sock, write(_, _)).WillOnce(SetErrnoAndReturn(ENETUNREACH, -1)); - EXPECT_FALSE(client->getConn().continueOnNetworkUnreachableDeadline); - client->start(&clientConnCallback); - EXPECT_TRUE(client->getConn().continueOnNetworkUnreachableDeadline); - ASSERT_FALSE(client->getConn().receivedNewPacketBeforeWrite); - ASSERT_TRUE(client->idleTimeout().isScheduled()); -} - -TEST_F( - QuicClientTransportTest, - NetworkUnreachableIsFatalIfContinueAfterDeadline) { - TransportSettings settings; - settings.continueOnNetworkUnreachable = true; - auto qLogger = std::make_shared(VantagePoint::Client); - client->getNonConstConn().qLogger = qLogger; - - client->setTransportSettings(settings); - client->addNewPeerAddress(serverAddr); - setupCryptoLayer(); - EXPECT_CALL(*sock, write(_, _)) - .WillRepeatedly(SetErrnoAndReturn(ENETUNREACH, -1)); - EXPECT_FALSE(client->getConn().continueOnNetworkUnreachableDeadline); - client->start(&clientConnCallback); - ASSERT_FALSE(client->getConn().receivedNewPacketBeforeWrite); - ASSERT_TRUE(client->idleTimeout().isScheduled()); - usleep(std::chrono::duration_cast( - settings.continueOnNetworkUnreachableDuration) - .count()); - EXPECT_CALL(clientConnCallback, onConnectionError(_)); - loopForWrites(); - - std::vector indices = - getQLogEventIndices(QLogEventType::TransportStateUpdate, qLogger); - EXPECT_EQ(indices.size(), 2); - - std::array updates = {kStart, kLossTimeoutExpired}; - for (int i = 0; i < 2; ++i) { - auto tmp = std::move(qLogger->logs[indices[i]]); - auto event = dynamic_cast(tmp.get()); - EXPECT_EQ(event->update, updates[i]); - } -} - -TEST_F( - QuicClientTransportTest, - NetworkUnreachableDeadlineIsResetAfterSuccessfulWrite) { - TransportSettings settings; - settings.continueOnNetworkUnreachable = true; - client->setTransportSettings(settings); - client->addNewPeerAddress(serverAddr); - EXPECT_CALL(clientConnCallback, onConnectionError(_)).Times(0); - setupCryptoLayer(); - EXPECT_CALL(*sock, write(_, _)) - .WillOnce(SetErrnoAndReturn(ENETUNREACH, -1)) - .WillOnce(Return(1)); - EXPECT_FALSE(client->getConn().continueOnNetworkUnreachableDeadline); - client->start(&clientConnCallback); - EXPECT_TRUE(client->getConn().continueOnNetworkUnreachableDeadline); - ASSERT_FALSE(client->getConn().receivedNewPacketBeforeWrite); - ASSERT_TRUE(client->idleTimeout().isScheduled()); - - client->lossTimeout().cancelTimeout(); - client->lossTimeout().timeoutExpired(); - EXPECT_FALSE(client->getConn().continueOnNetworkUnreachableDeadline); -} - TEST_F(QuicClientTransportTest, HappyEyeballsWithSingleV4Address) { auto& conn = client->getConn(); @@ -2337,10 +2264,9 @@ class QuicClientTransportHappyEyeballsTest : public QuicClientTransportTest { const SocketAddress& secondAddress) { auto& conn = client->getConn(); TransportSettings settings; - settings.continueOnNetworkUnreachable = true; client->setTransportSettings(settings); EXPECT_CALL(*sock, write(firstAddress, _)) - .WillOnce(SetErrnoAndReturn(ENETUNREACH, -1)); + .WillOnce(SetErrnoAndReturn(EAGAIN, -1)); EXPECT_CALL(*secondSock, write(_, _)); client->start(&clientConnCallback); EXPECT_EQ(conn.peerAddress, firstAddress); diff --git a/quic/state/StateData.h b/quic/state/StateData.h index d949d9eca..003dd7ce5 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -618,9 +618,6 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { // Error sent on the connection by the peer. folly::Optional> peerConnectionError; - // Before deadline, transport may treat ENETUNREACH as non-fatal error - folly::Optional continueOnNetworkUnreachableDeadline; - // Supported versions in order of preference. Only meaningful to clients. // TODO: move to client only conn state. std::vector supportedVersions; diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 245f98a03..372194e41 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -110,18 +110,6 @@ struct TransportSettings { // maximum number of packets we can batch. This does not apply to // BATCHING_MODE_NONE uint32_t maxBatchSize{kDefaultQuicMaxBatchSize}; - // Sets network unreachable to be a non fatal error. In some environments, - // EHOSTUNREACH or ENETUNREACH could just be because the routing table is - // being setup. This option makes those non fatal connection errors. - bool continueOnNetworkUnreachable{false}; - // Amount of time for which the transport treats ENETUNREACH/EHOSTUNREACH as - // non-fatal error since the first error is seen. If transport still sees the - // error after this amount of time, it'll throw and report the error. This is - // to minimize the negative impact on user experience for real no network - // case, so that errors are only delayed to be reported for 200ms, which - // should be invisible to end users. - // Choosing 150ms because loss timer fires at the 100ms for the first time. - std::chrono::milliseconds continueOnNetworkUnreachableDuration{150}; // Initial congestion window in MSS uint64_t initCwndInMss{kInitCwndInMss}; // Minimum congestion window in MSS