Reset is a unidirectional event

Summary:
Sending a reset shouldn't affect read states. Receiving a reset
shouldn't affect send states.

Reviewed By: afrind

Differential Revision: D15578287

fbshipit-source-id: 65c5e30666fd9e4c295317ba4c3e0653edbb78ff

quic/api/test/QuicTransportTest.cpp

@@ -1188,6 +1188,37 @@ TEST_F(QuicTransportTest, SendPathResponse) {
   EXPECT_TRUE(foundPathResponse);
 }
 
+TEST_F(QuicTransportTest, CloneAfterRecvReset) {
+  auto& conn = transport_->getConnectionState();
+  auto streamId = transport_->createBidirectionalStream().value();
+  transport_->writeChain(streamId, IOBuf::create(0), true, false);
+  loopForWrites();
+  EXPECT_EQ(1, conn.outstandingPackets.size());
+  auto stream = conn.streamManager->getStream(streamId);
+  EXPECT_EQ(1, stream->retransmissionBuffer.size());
+  EXPECT_EQ(0, stream->retransmissionBuffer.back().offset);
+  EXPECT_EQ(0, stream->retransmissionBuffer.back().data.chainLength());
+  EXPECT_TRUE(stream->retransmissionBuffer.back().eof);
+  EXPECT_TRUE(stream->lossBuffer.empty());
+  EXPECT_EQ(0, stream->writeBuffer.chainLength());
+  EXPECT_EQ(1, stream->currentWriteOffset);
+  EXPECT_EQ(0, *stream->finalWriteOffset);
+
+  RstStreamFrame rstFrame(streamId, GenericApplicationErrorCode::UNKNOWN, 0);
+  invokeStreamReceiveStateMachine(conn, *stream, std::move(rstFrame));
+
+  // This will clone twice. :/ Maybe we should change this to clone only once in
+  // the future, thus the EXPECT were written with LT and LE. But it will clone
+  // for sure and we shouldn't crash.
+  transport_->lossTimeout().timeoutExpired();
+  EXPECT_LT(1, conn.outstandingPackets.size());
+  size_t cloneCounter = std::count_if(
+      conn.outstandingPackets.begin(),
+      conn.outstandingPackets.end(),
+      [](const auto& packet) { return packet.associatedEvent.hasValue(); });
+  EXPECT_LE(1, cloneCounter);
+}
+
 TEST_F(QuicTransportTest, ClonePathResponse) {
   auto& conn = transport_->getConnectionState();
   // knock every handshake outstanding packets out

quic/client/test/QuicClientTransportTest.cpp

@@ -3500,7 +3500,8 @@ TEST_F(QuicClientTransportAfterStartTest, SendReset) {
 
   const auto& readCbs = client->getReadCallbacks();
   const auto& conn = client->getConn();
-  EXPECT_EQ(0, readCbs.count(streamId));
+  // ReadCallbacks are not affected by reseting send state
+  EXPECT_EQ(1, readCbs.count(streamId));
   // readable list can still be populated after a reset.
   EXPECT_FALSE(conn.streamManager->writableContains(streamId));
   auto packet = packetToBuf(createAckPacket(
@@ -3605,7 +3606,8 @@ TEST_F(QuicClientTransportAfterStartTest, SendResetAfterEom) {
   verifyShortPackets(sentPackets);
   const auto& readCbs = client->getReadCallbacks();
   const auto& conn = client->getConn();
-  EXPECT_EQ(readCbs.count(streamId), 0);
+  // ReadCallback are not affected by reseting send state.
+  EXPECT_EQ(1, readCbs.count(streamId));
   // readable list can still be populated after a reset.
   EXPECT_FALSE(conn.streamManager->writableContains(streamId));
 

quic/codec/QuicPacketRebuilder.cpp

@@ -84,10 +84,10 @@ folly::Optional<PacketEvent> PacketRebuilder::rebuildFromPacket(
             notPureAck |= ret;
             return ret;
           }
-          // If a stream is already Closed, or HalfClosedLocal, we should not
+          // If a stream is already Closed, we should not clone and resend this
-          // clone and resend this stream data. But should we abort the cloning
+          // stream data. But should we abort the cloning of this packet and
-          // of this packet and move on to the next packet? I'm gonna err on the
+          // move on to the next packet? I'm gonna err on the aggressive side
-          // aggressive side for now and call it success.
+          // for now and call it success.
           return true;
         },
         [&](const WriteCryptoFrame& cryptoFrame) {

quic/server/test/QuicServerTransportTest.cpp

@@ -1208,33 +1208,17 @@ TEST_F(QuicServerTransportTest, RecvRstStreamFrame) {
   auto packet = std::move(builder).buildPacket();
   deliverData(packetToBuf(packet));
 
-  // Verify pendingEvents on connection:
+  // Verify stream receive state is cleaned up but send state isn't:
-  EXPECT_TRUE(stream->streamWriteError.hasValue());
-  const RstStreamFrame* rstStreamFrame = nullptr;
-  for (auto& outstandingPacket : server->getNonConstConn().outstandingPackets) {
-    auto& frames = outstandingPacket.packet.frames;
-    for (auto& frame : frames) {
-      auto rstFramePtr = boost::get<RstStreamFrame>(&frame);
-      if (rstFramePtr && rstFramePtr->streamId == streamId) {
-        rstStreamFrame = rstFramePtr;
-        break;
-      }
-    }
-  }
-
-  // Verify stream state is cleaned up:
   auto updatedStream =
       server->getNonConstConn().streamManager->findStream(streamId);
   ASSERT_TRUE(updatedStream);
   EXPECT_TRUE(updatedStream->readBuffer.empty());
-  EXPECT_TRUE(updatedStream->writeBuffer.empty());
+  // We can verify retx buffer isn't empty here. The writeBuffer though could be
-  EXPECT_TRUE(updatedStream->retransmissionBuffer.empty());
+  // empty since deliverData can cause a write synchrously.
+  EXPECT_FALSE(updatedStream->retransmissionBuffer.empty());
   EXPECT_EQ(
       words.at(0).length() + words.at(1).length(),
       updatedStream->finalReadOffset.value());
-  EXPECT_EQ(
-      words.at(2).length() + words.at(3).length(),
-      updatedStream->currentWriteOffset);
   // updatedStream still writable since receiving rst has no impact on egress
   EXPECT_TRUE(updatedStream->writable());
 }

quic/state/stream/StreamStateFunctions.cpp

@@ -19,7 +19,6 @@ void resetQuicStream(QuicStreamState& stream, ApplicationErrorCode error) {
   stream.readBuffer.clear();
   stream.lossBuffer.clear();
   stream.streamWriteError = error;
-  stream.streamReadError = error;
   stream.conn.streamManager->updateReadableStreams(stream);
   stream.conn.streamManager->updateWritableStreams(stream);
   stream.conn.streamManager->updateLossStreams(stream);
@@ -41,17 +40,10 @@ void onResetQuicStream(QuicStreamState& stream, RstStreamFrame&& frame) {
   }
   // Verify that the flow control is consistent.
   updateFlowControlOnStreamData(stream, stream.maxOffsetObserved, frame.offset);
-  auto writeBufferLen = stream.writeBuffer.chainLength();
-  updateFlowControlOnWriteToSocket(stream, writeBufferLen);
   // Drop read buffer:
   stream.readBuffer.clear();
-  // We can also drop retx buffer and writeBuffer:
-  stream.retransmissionBuffer.clear();
-  stream.writeBuffer.clear();
-  stream.lossBuffer.clear();
   stream.finalReadOffset = frame.offset;
   stream.streamReadError = frame.errorCode;
-  stream.streamWriteError = frame.errorCode;
   bool appReadAllBytes = stream.finalReadOffset &&
       stream.currentReadOffset > *stream.finalReadOffset;
   if (!appReadAllBytes) {

quic/state/stream/test/StreamStateFunctionsTest.cpp

@@ -19,7 +19,7 @@ namespace test {
 
 class StreamStateFunctionsTests : public Test {};
 
-TEST_F(StreamStateFunctionsTests, SanityTest) {
+TEST_F(StreamStateFunctionsTests, BasicResetTest) {
   QuicServerConnectionState conn;
   StreamId streamId = 0xbaad;
   QuicStreamState stream(streamId, conn);
@@ -44,7 +44,6 @@ TEST_F(StreamStateFunctionsTests, SanityTest) {
   EXPECT_TRUE(stream.writeBuffer.empty());
   EXPECT_TRUE(stream.retransmissionBuffer.empty());
   EXPECT_TRUE(stream.readBuffer.empty());
-  EXPECT_TRUE(stream.streamReadError.hasValue());
 
   // The rest are untouched:
   EXPECT_EQ(stream.id, streamId);