Add open_streams metric

Summary: I want to have visibility in to the current count of streams. There are already callback methods onNew/onClosed, unfortunately, the latter is not called when connections are closed, so I had to fix that to prevent the metric from uncontrolled growth. I am still not positive it's correct and was thinking of hooking into QuicStreamState destructor instead.

Reviewed By: kvtsoy

Differential Revision: D56195487

fbshipit-source-id: 901bce9473327a8f7ef1bc0f70af10899784d681

quic/api/QuicTransportBase.cpp

@@ -387,11 +387,6 @@ void QuicTransportBase::closeImpl(
 
   // Clear out all the streams, we don't need them any more. When the peer
   // receives the conn close they will implicitly reset all the streams.
-  QUIC_STATS_FOR_EACH(
-      conn_->streamManager->streams().cbegin(),
-      conn_->streamManager->streams().cend(),
-      conn_->statsCallback,
-      onQuicStreamClosed);
   conn_->streamManager->clearOpenStreams();
 
   // Clear out all the buffered datagrams

quic/api/test/QuicTransportBaseTest.cpp

@@ -22,6 +22,7 @@
 #include <quic/state/QuicStreamUtilities.h>
 #include <quic/state/stream/StreamReceiveHandlers.h>
 #include <quic/state/stream/StreamSendHandlers.h>
+#include <quic/state/test/MockQuicStats.h>
 #include <quic/state/test/Mocks.h>
 
 #include <quic/common/testutil/MockAsyncUDPSocket.h>
@@ -4289,6 +4290,26 @@ TEST_P(QuicTransportImplTestBase, BackgroundModeChangeWithStreamChanges) {
   manager.removeClosedStream(stream2Id);
 }
 
+class QuicTransportImplTestCounters : public QuicTransportImplTest {};
+
+TEST_F(QuicTransportImplTestCounters, TransportResetClosesStreams) {
+  MockQuicStats quicStats;
+  auto transportSettings = transport->getTransportSettings();
+  auto& conn = transport->getConnectionState();
+  conn.statsCallback = &quicStats;
+
+  EXPECT_CALL(quicStats, onNewQuicStream()).Times(2);
+  EXPECT_CALL(quicStats, onQuicStreamClosed()).Times(2);
+
+  auto stream1 = transport->createBidirectionalStream().value();
+  auto stream2 = transport->createBidirectionalStream().value();
+
+  EXPECT_EQ(stream1, 1);
+  EXPECT_EQ(stream2, 5);
+  EXPECT_EQ(conn.streamManager->streamCount(), 2);
+  transport.reset();
+}
+
 class QuicTransportImplTestWithGroups : public QuicTransportImplTestBase {};
 
 INSTANTIATE_TEST_SUITE_P(

quic/api/test/QuicTransportFunctionsTest.cpp

@@ -4757,5 +4757,45 @@ TEST_F(
       expectedSignal.maybeBytesToSend, congestionControlWritableBytes(*conn));
 }
 
+TEST_F(
+    QuicTransportFunctionsTest,
+    onQuicStreamClosedNotCalledOnStreamClearing) {
+  {
+    auto conn = createConn();
+    EXPECT_CALL(*quicStats_, onNewQuicStream()).Times(1);
+    EXPECT_CALL(*quicStats_, onQuicStreamClosed()).Times(1);
+    auto stream = conn->streamManager->createNextBidirectionalStream().value();
+    EXPECT_EQ(stream->id, 1);
+    EXPECT_EQ(conn->streamManager->streamCount(), 1);
+    conn->streamManager->clearOpenStreams();
+    EXPECT_EQ(conn->streamManager->streamCount(), 0);
+  }
+}
+
+TEST_F(QuicTransportFunctionsTest, onQuicStreamClosedCalledOnConnClosure) {
+  {
+    auto conn = createConn();
+    EXPECT_CALL(*quicStats_, onNewQuicStream()).Times(1);
+    EXPECT_CALL(*quicStats_, onQuicStreamClosed()).Times(1);
+    auto stream = conn->streamManager->createNextBidirectionalStream().value();
+    EXPECT_EQ(stream->id, 1);
+    EXPECT_EQ(conn->streamManager->streamCount(), 1);
+    conn.reset();
+  }
+}
+
+TEST_F(QuicTransportFunctionsTest, onQuicStreamClosed) {
+  auto conn = createConn();
+  EXPECT_CALL(*quicStats_, onNewQuicStream()).Times(1);
+  EXPECT_CALL(*quicStats_, onQuicStreamClosed()).Times(1);
+  auto stream = conn->streamManager->createNextBidirectionalStream().value();
+  EXPECT_EQ(stream->id, 1);
+  EXPECT_EQ(conn->streamManager->streamCount(), 1);
+  stream->sendState = StreamSendState::Closed;
+  stream->recvState = StreamRecvState::Closed;
+  conn->streamManager->removeClosedStream(stream->id);
+  EXPECT_EQ(conn->streamManager->streamCount(), 0);
+}
+
 } // namespace test
 } // namespace quic

quic/server/state/ServerStateMachine.h

@@ -89,7 +89,9 @@ struct MaxPacingRateKnobState {
 };
 
 struct QuicServerConnectionState : public QuicConnectionStateBase {
-  ~QuicServerConnectionState() override = default;
+  ~QuicServerConnectionState() override {
+    streamManager->clearOpenStreams();
+  }
 
   ServerState state;
 

quic/state/QuicStreamManager.cpp

@@ -7,6 +7,7 @@
 
 #include <quic/state/QuicStreamManager.h>
 #include <quic/state/QuicStreamUtilities.h>
+#include <quic/state/QuicTransportStatsCallback.h>
 #include <quic/state/StateData.h>
 
 namespace quic {
@@ -779,4 +780,22 @@ void QuicStreamManager::addToStreamPriorityMap(
   notifyStreamPriorityChanges();
 }
 
+void QuicStreamManager::clearOpenStreams() {
+  QUIC_STATS_FOR_EACH(
+      streams().cbegin(),
+      streams().cend(),
+      conn_.statsCallback,
+      onQuicStreamClosed);
+
+  openBidirectionalLocalStreams_.clear();
+  openUnidirectionalLocalStreams_.clear();
+  openBidirectionalPeerStreams_.clear();
+  openUnidirectionalPeerStreams_.clear();
+  openBidirectionalLocalStreamGroups_.clear();
+  openUnidirectionalLocalStreamGroups_.clear();
+  peerUnidirectionalStreamGroupsSeen_.clear();
+  peerBidirectionalStreamGroupsSeen_.clear();
+  streams_.clear();
+}
+
 } // namespace quic

quic/state/QuicStreamManager.h

@@ -406,17 +406,7 @@ class QuicStreamManager {
   /*
    * Clear all the currently open streams.
    */
-  void clearOpenStreams() {
+  void clearOpenStreams();
-    openBidirectionalLocalStreams_.clear();
-    openUnidirectionalLocalStreams_.clear();
-    openBidirectionalPeerStreams_.clear();
-    openUnidirectionalPeerStreams_.clear();
-    openBidirectionalLocalStreamGroups_.clear();
-    openUnidirectionalLocalStreamGroups_.clear();
-    peerUnidirectionalStreamGroupsSeen_.clear();
-    peerBidirectionalStreamGroupsSeen_.clear();
-    streams_.clear();
-  }
 
   /*
    * Return a const reference to the underlying container holding the stream