From c826dd592bf917b1f377fc3fa859d1b0f2fb8f30 Mon Sep 17 00:00:00 2001
From: Matt Joras <mjoras@fb.com>
Date: Mon, 6 Apr 2020 10:12:37 -0700
Subject: [PATCH] Do not checkForClosedStream from read.

Summary:
This is a pretty confusing API behavior. This means that when reading from stream A, stream B could potentially be removed if it doesn't have a read callback set. This effectively means you cannot safely call read from onNew*Stream.

There's no real reason the check for a closed stream here, as the streams will get cleaned up from the onNetworkData or read looper calls.

Reviewed By: yangchi

Differential Revision: D20848090

fbshipit-source-id: 38eba9e5a24b6ddf5bf05ac75787dda9c001c1c6
---
 quic/api/QuicTransportBase.cpp          |  3 ---
 quic/api/test/QuicTransportBaseTest.cpp | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp
index 6bcc00e67..b40eb3ac2 100644
--- a/quic/api/QuicTransportBase.cpp
+++ b/quic/api/QuicTransportBase.cpp
@@ -1255,7 +1255,6 @@ folly::Expected<std::pair<Buf, bool>, LocalErrorCode> QuicTransportBase::read(
   }
   FOLLY_MAYBE_UNUSED auto self = sharedGuard();
   SCOPE_EXIT {
-    checkForClosedStream();
     updateReadLooper();
     updatePeekLooper(); // read can affect "peek" API
     updateWriteLooper(true);
@@ -1317,7 +1316,6 @@ folly::Expected<folly::Unit, LocalErrorCode> QuicTransportBase::peek(
   }
   FOLLY_MAYBE_UNUSED auto self = sharedGuard();
   SCOPE_EXIT {
-    checkForClosedStream();
     updatePeekLooper();
     updateWriteLooper(true);
   };
@@ -1375,7 +1373,6 @@ folly::
   }
   FOLLY_MAYBE_UNUSED auto self = sharedGuard();
   SCOPE_EXIT {
-    checkForClosedStream();
     updatePeekLooper();
     updateReadLooper(); // consume may affect "read" API
     updateWriteLooper(true);
diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp
index 314f60b81..78383030c 100644
--- a/quic/api/test/QuicTransportBaseTest.cpp
+++ b/quic/api/test/QuicTransportBaseTest.cpp
@@ -829,6 +829,25 @@ TEST_F(QuicTransportImplTest, onNewBidirectionalStreamCallback) {
   transport.reset();
 }
 
+TEST_F(QuicTransportImplTest, onNewStreamCallbackDoesNotRemove) {
+  auto readData = folly::IOBuf::copyBuffer("actual stream data");
+  StreamId uniStream1 = 2;
+  StreamId uniStream2 = uniStream1 + kStreamIncrement;
+  EXPECT_CALL(connCallback, onNewUnidirectionalStream(uniStream1))
+      .WillOnce(Invoke([&](StreamId id) {
+        ASSERT_FALSE(transport->read(id, 100).hasError());
+      }));
+  EXPECT_CALL(connCallback, onNewUnidirectionalStream(uniStream2))
+      .WillOnce(Invoke([&](StreamId id) {
+        ASSERT_FALSE(transport->read(id, 100).hasError());
+      }));
+  transport->addDataToStream(
+      uniStream1, StreamBuffer(readData->clone(), 0, true));
+  transport->addDataToStream(
+      uniStream2, StreamBuffer(readData->clone(), 0, true));
+  transport.reset();
+}
+
 TEST_F(QuicTransportImplTest, onNewBidirectionalStreamStreamOutOfOrder) {
   InSequence dummy;
   auto readData = folly::IOBuf::copyBuffer("actual stream data");