diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index bf14ea8bd..9c2435ebd 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/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 diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index e27f268be..4f95ab0ab 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/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)); diff --git a/quic/codec/QuicPacketRebuilder.cpp b/quic/codec/QuicPacketRebuilder.cpp index cc505d2ec..04c566d49 100644 --- a/quic/codec/QuicPacketRebuilder.cpp +++ b/quic/codec/QuicPacketRebuilder.cpp @@ -84,10 +84,10 @@ folly::Optional PacketRebuilder::rebuildFromPacket( notPureAck |= ret; return ret; } - // If a stream is already Closed, or HalfClosedLocal, we should not - // clone and resend this stream data. But should we abort the cloning - // of this packet and move on to the next packet? I'm gonna err on the - // aggressive side for now and call it success. + // If a stream is already Closed, we should not clone and resend this + // stream data. But should we abort the cloning of this packet and + // move on to the next packet? I'm gonna err on the aggressive side + // for now and call it success. return true; }, [&](const WriteCryptoFrame& cryptoFrame) { diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 912b1a842..8a55300bc 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/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: - 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(&frame); - if (rstFramePtr && rstFramePtr->streamId == streamId) { - rstStreamFrame = rstFramePtr; - break; - } - } - } - - // Verify stream state is cleaned up: + // Verify stream receive state is cleaned up but send state isn't: auto updatedStream = server->getNonConstConn().streamManager->findStream(streamId); ASSERT_TRUE(updatedStream); EXPECT_TRUE(updatedStream->readBuffer.empty()); - EXPECT_TRUE(updatedStream->writeBuffer.empty()); - EXPECT_TRUE(updatedStream->retransmissionBuffer.empty()); + // We can verify retx buffer isn't empty here. The writeBuffer though could be + // 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()); } diff --git a/quic/state/stream/StreamStateFunctions.cpp b/quic/state/stream/StreamStateFunctions.cpp index 37e78e562..354061916 100644 --- a/quic/state/stream/StreamStateFunctions.cpp +++ b/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) { diff --git a/quic/state/stream/test/StreamStateFunctionsTest.cpp b/quic/state/stream/test/StreamStateFunctionsTest.cpp index a59d353cd..30ee50802 100644 --- a/quic/state/stream/test/StreamStateFunctionsTest.cpp +++ b/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);