Properly close DSR streams on ACK.

Summary: The bug here is that we weren't accounting for the DSR state when checking if the stream should be closed. This causes the stream state to live until the connection closes.

Reviewed By: yairgott

Differential Revision: D36269711

fbshipit-source-id: 403268db1940b2f00130709243aa6df1d2af39cd

quic/state/QuicStreamFunctions.cpp

@@ -386,10 +386,10 @@ bool allBytesTillFinAcked(const QuicStreamState& stream) {
    * 4. We have no bytes left to write
    * 5. We have no bytes that are detected as lost.
    */
-  return stream.finalWriteOffset &&
+  return stream.hasSentFIN() && stream.retransmissionBuffer.empty() &&
-      stream.currentWriteOffset > *stream.finalWriteOffset &&
+      stream.retransmissionBufMetas.empty() && stream.writeBuffer.empty() &&
-      stream.retransmissionBuffer.empty() && stream.writeBuffer.empty() &&
+      !stream.hasWritableBufMeta() && stream.lossBuffer.empty() &&
-      stream.lossBuffer.empty();
+      stream.lossBufMetas.empty();
 }
 
 void appendPendingStreamReset(

quic/state/stream/test/StreamStateMachineTest.cpp

@@ -854,6 +854,45 @@ TEST_F(QuicOpenStateTest, DSRStreamAcked) {
   frame.fromBufMeta = true;
   sendAckSMHandler(*stream, frame);
   EXPECT_TRUE(stream->retransmissionBufMetas.empty());
+  EXPECT_EQ(stream->sendState, StreamSendState::Open);
+}
+
+TEST_F(QuicOpenStateTest, DSRFullStreamAcked) {
+  auto conn = createConn();
+  conn->clientConnectionId = getTestConnectionId(0);
+  conn->serverConnectionId = getTestConnectionId(1);
+  auto stream = conn->streamManager->createNextBidirectionalStream().value();
+  auto buf = folly::IOBuf::copyBuffer("Big ship stucks in small water");
+  size_t len = buf->computeChainDataLength();
+  writeDataToQuicStream(*stream, std::move(buf), false);
+  handleStreamWritten(
+      *conn, *stream, 0, len, false, 1, PacketNumberSpace::AppData);
+  ASSERT_EQ(stream->retransmissionBuffer.size(), 1);
+  writeBufMetaToQuicStream(*stream, BufferMeta(1000), true);
+  auto bufMetaStartingOffset = stream->writeBufMeta.offset;
+  handleStreamBufMetaWritten(
+      *conn,
+      *stream,
+      bufMetaStartingOffset,
+      1000,
+      true,
+      1,
+      PacketNumberSpace::AppData);
+  ASSERT_EQ(stream->writeBuffer.chainLength(), 0);
+  ASSERT_NE(
+      stream->retransmissionBufMetas.end(),
+      stream->retransmissionBufMetas.find(bufMetaStartingOffset));
+  WriteStreamFrame frame(stream->id, bufMetaStartingOffset, 1000, true);
+  frame.fromBufMeta = true;
+  sendAckSMHandler(*stream, frame);
+  frame.offset = 0;
+  frame.len = len;
+  frame.fin = false;
+  frame.fromBufMeta = false;
+  sendAckSMHandler(*stream, frame);
+  EXPECT_TRUE(stream->retransmissionBuffer.empty());
+  EXPECT_TRUE(stream->retransmissionBufMetas.empty());
+  EXPECT_EQ(stream->sendState, StreamSendState::Closed);
 }
 
 } // namespace test

quic/state/test/QuicStreamFunctionsTest.cpp

@@ -1998,6 +1998,14 @@ TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAcked) {
   EXPECT_TRUE(allBytesTillFinAcked(stream));
 }
 
+TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedDSR) {
+  StreamId id = 3;
+  QuicStreamState stream(id, conn);
+  stream.finalWriteOffset = 1;
+  stream.writeBufMeta.offset = 2;
+  EXPECT_TRUE(allBytesTillFinAcked(stream));
+}
+
 TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedFinOnly) {
   StreamId id = 3;
   QuicStreamState stream(id, conn);
@@ -2006,6 +2014,14 @@ TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedFinOnly) {
   EXPECT_TRUE(allBytesTillFinAcked(stream));
 }
 
+TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedFinOnlyDSR) {
+  StreamId id = 3;
+  QuicStreamState stream(id, conn);
+  stream.finalWriteOffset = 0;
+  stream.writeBufMeta.offset = 1;
+  EXPECT_TRUE(allBytesTillFinAcked(stream));
+}
+
 TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedNewStream) {
   StreamId id = 3;
   QuicStreamState stream(id, conn);
@@ -2021,6 +2037,19 @@ TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillLost) {
   EXPECT_FALSE(allBytesTillFinAcked(stream));
 }
 
+TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillLostDSR) {
+  StreamId id = 3;
+  QuicStreamState stream(id, conn);
+  stream.finalWriteOffset = 20;
+  stream.writeBufMeta.offset = 21;
+  WriteBufferMeta::Builder b;
+  b.setLength(10);
+  b.setOffset(10);
+  b.setEOF(false);
+  stream.lossBufMetas.emplace_back(b.build());
+  EXPECT_FALSE(allBytesTillFinAcked(stream));
+}
+
 TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillRetransmitting) {
   StreamId id = 3;
   QuicStreamState stream(id, conn);
@@ -2030,6 +2059,18 @@ TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillRetransmitting) {
   EXPECT_FALSE(allBytesTillFinAcked(stream));
 }
 
+TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillRetransmittingDSR) {
+  StreamId id = 3;
+  QuicStreamState stream(id, conn);
+  stream.finalWriteOffset = 12;
+  WriteBufferMeta::Builder b;
+  b.setLength(10);
+  b.setOffset(10);
+  b.setEOF(false);
+  stream.retransmissionBufMetas.emplace(0, b.build());
+  EXPECT_FALSE(allBytesTillFinAcked(stream));
+}
+
 TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillWriting) {
   StreamId id = 3;
   QuicStreamState stream(id, conn);
@@ -2040,6 +2081,14 @@ TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillWriting) {
   EXPECT_FALSE(allBytesTillFinAcked(stream));
 }
 
+TEST_F(QuicStreamFunctionsTest, AllBytesTillFinAckedStillWritingDSR) {
+  StreamId id = 3;
+  QuicStreamState stream(id, conn);
+  stream.finalWriteOffset = 10;
+  stream.writeBufMeta.length = 10;
+  EXPECT_FALSE(allBytesTillFinAcked(stream));
+}
+
 TEST_F(QuicServerStreamFunctionsTest, TestAppendPendingStreamResetAllData) {
   StreamId id = 3;
   QuicStreamState stream(id, conn);