TODO comment cleanup.

Summary:
These are either no longer relevant, are unlikely to be done, or are spculative enough that they don't deserve code space.

Hope here is to make our search for TODOs higher signal.

Reviewed By: lnicco

Differential Revision: D29769792

fbshipit-source-id: 7cfa62cdc15e72d8b7b0cd5dbb5913ea3ca3dc5a

quic/QuicConstants.h

@@ -438,8 +438,6 @@ constexpr uint16_t kDefaultRxPacketsBeforeAckBeforeInit = 10;
 constexpr uint16_t kDefaultRxPacketsBeforeAckAfterInit = 10;
 
 /* Ack timer */
-// TODO: These numbers are shamlessly taken from Chromium code. We have no idea
-// how good/bad this is.
 // Ack timeout = SRTT * kAckTimerFactor
 constexpr double kAckTimerFactor = 0.25;
 // max ack timeout: 25ms

quic/api/IoBufQuicBatch.cpp

@@ -97,9 +97,6 @@ bool IOBufQuicBatch::flushInternal() {
 
   folly::Optional<int> secondSocketErrno;
   if (happyEyeballsState_.shouldWriteToSecondSocket) {
-    // TODO: if the errno is EMSGSIZE, and we move on with the second socket,
-    // we actually miss the chance to fix our UDP packet size with the first
-    // socket.
     auto consumed = batchWriter_->write(
         *happyEyeballsState_.secondSocket,
         happyEyeballsState_.secondPeerAddress);
@@ -160,9 +157,8 @@ bool IOBufQuicBatch::flushInternal() {
   }
 
   if (!written) {
-    // This can happen normally, so ignore for now. Now we treat EAGAIN same
+    // This can happen normally, so ignore. Now we treat most errors same
     // as a loss to avoid looping.
-    // TODO: Remove once we use write event from libevent.
     return false; // done
   }
 

quic/api/QuicPacketScheduler.cpp

@@ -242,7 +242,7 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket(
   if (rstScheduler_ && rstScheduler_->hasPendingRsts()) {
     rstWritten = rstScheduler_->writeRsts(wrapper);
   }
-  // TODO: Long time ago we decided RST has higher priority than Acks. Why tho?
+  // Long time ago we decided RST has higher priority than Acks.
   if (ackScheduler_ && ackScheduler_->hasPendingAcks()) {
     if (cryptoDataWritten || rstWritten) {
       // If packet has non ack data, it is subject to congestion control. We
@@ -570,7 +570,6 @@ bool RstStreamScheduler::hasPendingRsts() const {
 bool RstStreamScheduler::writeRsts(PacketBuilderInterface& builder) {
   bool rstWritten = false;
   for (const auto& resetStream : conn_.pendingEvents.resets) {
-    // TODO: here, maybe coordinate scheduling of RST_STREAMS and streams.
     auto bytesWritten = writeFrame(resetStream.second, builder);
     if (!bytesWritten) {
       break;

quic/api/QuicPacketScheduler.h

@@ -323,9 +323,6 @@ class CloningScheduler : public QuicPacketScheduler {
  public:
   // Normally a scheduler takes in a const conn, and update conn later. But for
   // this one I want to update conn right inside this class itself.
-  // TODO: Passing cipherOverhead into the CloningScheduler to recalculate the
-  // correct writableBytes isn't ideal. But unblock me or others from quickly
-  // testing it on load test. :(
   CloningScheduler(
       FrameScheduler& scheduler,
       QuicConnectionStateBase& conn,

quic/api/QuicTransportBase.cpp

@@ -206,7 +206,6 @@ void QuicTransportBase::closeNow(
   // the drain timeout may have been scheduled by a previous close, in which
   // case, our close would not take effect. This cancels the drain timeout in
   // this case and expires the timeout.
-  // TODO: fix this in a better way.
   if (drainTimeout_.isScheduled()) {
     drainTimeout_.cancelTimeout();
     drainTimeoutExpired();
@@ -365,7 +364,6 @@ void QuicTransportBase::closeImpl(
   peekLooper_->stop();
   writeLooper_->stop();
 
-  // TODO: invoke connection close callbacks.
   cancelAllAppCallbacks(cancelCode);
 
   // Clear out all the pending events, we don't need them any more.
@@ -1487,8 +1485,6 @@ void QuicTransportBase::processCallbacksAfterNetworkData() {
 
   handleKnobCallbacks();
 
-  // TODO: we're currently assuming that canceling write callbacks will not
-  // cause reset of random streams. Maybe get rid of that assumption later.
   for (auto pendingResetIt = conn_->pendingEvents.resets.begin();
        pendingResetIt != conn_->pendingEvents.resets.end();
        pendingResetIt++) {
@@ -1589,8 +1585,7 @@ void QuicTransportBase::processCallbacksAfterNetworkData() {
     }
 
     // If the connection flow control is unblocked, we might be unblocked
-    // on the streams now. TODO: maybe do this only when we know connection
-    // flow control changed.
+    // on the streams now.
     auto writeCallbackIt = pendingWriteCallbacks_.begin();
 
     while (writeCallbackIt != pendingWriteCallbacks_.end()) {
@@ -2293,7 +2288,6 @@ void QuicTransportBase::lossTimeoutExpired() noexcept {
   FOLLY_MAYBE_UNUSED auto self = sharedGuard();
   try {
     onLossDetectionAlarm(*conn_, markPacketLoss);
-    // TODO: remove this trace when Pacing is ready to land
     if (conn_->qLogger) {
       conn_->qLogger->addTransportStateUpdate(kLossTimeoutExpired);
     }

quic/api/QuicTransportFunctions.cpp

@@ -103,9 +103,6 @@ WriteQuicDataResult writeQuicDataToSocketImpl(
     uint64_t packetLimit,
     bool exceptCryptoStream) {
   auto builder = ShortHeaderBuilder();
-  // TODO: In FrameScheduler, Retx is prioritized over new data. We should
-  // add a flag to the Scheduler to control the priority between them and see
-  // which way is better.
   WriteQuicDataResult result;
   auto& packetsWritten = result.packetsWritten;
   auto& probesWritten = result.probesWritten;
@@ -1203,7 +1200,6 @@ void writeCloseCommon(
            << " sent close packetNum=" << packetNum << " in space=" << pnSpace
            << " " << connection;
   // Increment the sequence number.
-  // TODO: Do not increase pn if write fails
   increaseNextPacketNum(connection, pnSpace);
   // best effort writing to the socket, ignore any errors.
   auto ret = sock.write(connection.peerAddress, packetBuf);
@@ -1357,7 +1353,6 @@ uint64_t writeConnectionDataToSocket(
       writableBytes -= cipherOverhead;
     }
 
-    // TODO: Select a different DataPathFunc based on TransportSettings
     const auto& dataPlainFunc =
         connection.transportSettings.dataPathType == DataPathType::ChainedMemory
         ? iobufChainBasedBuildScheduleEncrypt

quic/api/QuicTransportFunctions.h

@@ -148,7 +148,6 @@ uint64_t writeZeroRttDataToSocket(
 /**
  * Whether we should and can write data.
  *
- * TODO: We should probably split "should" and "can" into two APIs.
  */
 WriteDataReason shouldWriteData(const QuicConnectionStateBase& conn);
 bool hasAckDataToWrite(const QuicConnectionStateBase& conn);

quic/client/QuicClientTransport.cpp

@@ -83,9 +83,6 @@ QuicClientTransport::QuicClientTransport(
 
   conn_->readCodec->setCodecParameters(CodecParameters(
       conn_->peerAckDelayExponent, conn_->originalVersion.value()));
-  // TODO: generate this once we can generate the packet sequence number
-  // correctly.
-  // conn_->nextSequenceNum = folly::Random::secureRandom<PacketNum>();
 
   VLOG(10) << "client created " << *conn_;
 }
@@ -331,9 +328,6 @@ void QuicClientTransport::processPacketData(
 
   // We got a packet that was not the version negotiation packet, that means
   // that the version is now bound to the new packet.
-  // TODO: move this into the state machine.
-  // TODO: get this from the crypto layer instead. This would be a security vuln
-  // if we don't.
   if (!conn_->version) {
     conn_->version = conn_->originalVersion;
   }
@@ -752,8 +746,6 @@ void QuicClientTransport::onReadData(
     NetworkDataSingle&& networkData) {
   if (closeState_ == CloseState::CLOSED) {
     // If we are closed, then we shoudn't process new network data.
-    // TODO: we might want to process network data if we decide that we should
-    // exit draining state early
     QUIC_STATS(
         statsCallback_, onPacketDropped, PacketDropReason::CLIENT_STATE_CLOSED);
     if (conn_->qLogger) {
@@ -791,9 +783,6 @@ void QuicClientTransport::onReadData(
 }
 
 void QuicClientTransport::writeData() {
-  // TODO: replace with write in state machine.
-  // TODO: change to draining when we move the client to have a draining state
-  // as well.
   QuicVersion version = conn_->version.value_or(*conn_->originalVersion);
   const ConnectionId& srcConnId = *conn_->clientConnectionId;
   const ConnectionId* destConnId =
@@ -950,11 +939,7 @@ void QuicClientTransport::writeData() {
 
 void QuicClientTransport::startCryptoHandshake() {
   auto self = this->shared_from_this();
-  // Set idle timer whenever crypto starts so that we can restart the idle timer
-  // after a version negotiation as well.
   setIdleTimer();
-  // TODO: no need to close the transport if there is an error in the
-  // handshake.
   // We need to update the flow control settings every time we start a crypto
   // handshake. This is so that we can reset the flow control settings when
   // we go through version negotiation as well.

quic/client/state/ClientStateMachine.cpp

@@ -150,7 +150,6 @@ void processServerInitialParams(
       maxStreamDataBidiRemote.value_or(0);
   conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetUni =
       maxStreamDataUni.value_or(0);
-  // TODO Make idleTimeout disableable via transport parameter.
   conn.streamManager->setMaxLocalBidirectionalStreams(
       maxStreamsBidi.value_or(0));
   conn.peerAdvertisedInitialMaxStreamsBidi = maxStreamsBidi.value_or(0);
@@ -166,7 +165,6 @@ void processServerInitialParams(
   }
   conn.peerAckDelayExponent =
       ackDelayExponent.value_or(kDefaultAckDelayExponent);
-  // TODO: udpSendPacketLen should also be limited by PMTU
   if (conn.transportSettings.canIgnorePathMTU) {
     if (*packetSize > kDefaultMaxUDPPayload) {
       *packetSize = kDefaultUDPSendPacketLen;

quic/client/state/ClientStateMachine.h

@@ -77,8 +77,6 @@ struct QuicClientConnectionState : public QuicConnectionStateBase {
         handshakeFactory(std::move(handshakeFactoryIn)) {
     cryptoState = std::make_unique<QuicCryptoState>();
     congestionController = std::make_unique<Cubic>(*this);
-    // TODO: this is wrong, it should be the handshake finish time. But i need
-    // a relatively sane time now to make the timestamps all sane.
     connectionTime = Clock::now();
     originalVersion = QuicVersion::MVFST;
     DCHECK(handshakeFactory);

quic/codec/Decode.cpp

@@ -569,7 +569,6 @@ NewConnectionIdFrame decodeNewConnectionIdFrame(folly::io::Cursor& cursor) {
 
 RetireConnectionIdFrame decodeRetireConnectionIdFrame(
     folly::io::Cursor& cursor) {
-  // TODO we parse this frame, but return NoopFrame. Add proper support for it!
   auto sequenceNum = decodeQuicInteger(cursor);
   if (!sequenceNum) {
     throw QuicTransportException(
@@ -746,7 +745,6 @@ QuicFrame parseFrame(
     const CodecParameters& params) {
   folly::io::Cursor cursor(queue.front());
   auto frameTypeInt = decodeQuicInteger(cursor);
-  // TODO add an new api to determine whether the frametype is encoded minimally
   if (!frameTypeInt) {
     throw QuicTransportException(
         "Invalid frame-type field", TransportErrorCode::FRAME_ENCODING_ERROR);

quic/codec/QuicPacketBuilder.h

@@ -36,7 +36,6 @@ constexpr auto kLongHeaderHeaderSize = sizeof(uint8_t) /* Type bytes */ +
     kReservedPacketLenSize /* minimal size of length */ +
     kReservedPacketNumSize /* packet number */;
 
-// TODO: i'm sure this isn't the optimal value:
 // Appender growth byte size for in PacketBuilder:
 constexpr size_t kAppenderGrowthSize = 100;
 

quic/codec/QuicReadCodec.cpp

@@ -168,7 +168,6 @@ CodecResult QuicReadCodec::parseLongHeaderPacket(
         CipherUnavailable(std::move(currentPacketData), 0, protectionType));
   }
 
-  // TODO: decrypt the long header.
   PacketNum expectedNextPacketNum = 0;
   folly::Optional<PacketNum> largestReceivedPacketNum;
   switch (longHeaderTypeToProtectionType(type)) {

quic/codec/QuicWriteCodec.cpp

@@ -262,8 +262,6 @@ folly::Optional<AckFrameWriteResult> writeAckFrame(
   WriteAckFrame ackFrame;
   uint64_t spaceLeft = builder.remainingSpaceInPkt();
   uint64_t beginningSpace = spaceLeft;
-  // Reserve enough space a full packet of ACKs with 2 byte varints.
-  // TODO is this a good heuristic?
   ackFrame.ackBlocks.reserve(spaceLeft / 4);
 
   // We could technically split the range if the size of the representation of

quic/codec/Types.h

@@ -226,8 +226,6 @@ struct ReadCryptoFrame {
       : offset(offsetIn), data(folly::IOBuf::create(0)) {}
 
   // Stuff stored in a variant type needs to be copyable.
-  // TODO: can we make this copyable only by the variant, but not
-  // by anyone else.
   ReadCryptoFrame(const ReadCryptoFrame& other) {
     offset = other.offset;
     if (other.data) {
@@ -278,8 +276,6 @@ struct ReadNewTokenFrame {
   ReadNewTokenFrame(Buf tokenIn) : token(std::move(tokenIn)) {}
 
   // Stuff stored in a variant type needs to be copyable.
-  // TODO: can we make this copyable only by the variant, but not
-  // by anyone else.
   ReadNewTokenFrame(const ReadNewTokenFrame& other) {
     if (other.token) {
       token = other.token->clone();
@@ -367,8 +363,6 @@ struct ReadStreamFrame {
         fin(finIn) {}
 
   // Stuff stored in a variant type needs to be copyable.
-  // TODO: can we make this copyable only by the variant, but not
-  // by anyone else.
   ReadStreamFrame(const ReadStreamFrame& other) {
     streamId = other.streamId;
     offset = other.offset;
@@ -724,7 +718,6 @@ struct LongHeaderInvariant {
   LongHeaderInvariant(QuicVersion ver, ConnectionId scid, ConnectionId dcid);
 };
 
-// TODO: split this into read and write types.
 struct LongHeader {
  public:
   virtual ~LongHeader() = default;

quic/congestion_control/Bbr.cpp

@@ -37,7 +37,6 @@ BbrCongestionController::BbrCongestionController(QuicConnectionStateBase& conn)
           conn.udpSendPacketLen * conn.transportSettings.maxCwndInMss),
       pacingWindow_(
           conn.udpSendPacketLen * conn.transportSettings.initCwndInMss),
-      // TODO: experiment with longer window len for ack aggregation filter
       maxAckHeightFilter_(kBandwidthWindowLength, 0, 0) {}
 
 CongestionControlType BbrCongestionController::type() const noexcept {
@@ -81,8 +80,6 @@ void BbrCongestionController::onPacketLoss(
     // We need to make sure CONSERVATIVE can last for a round trip, so update
     // endOfRoundTrip_ to the latest sent packet.
     endOfRoundTrip_ = Clock::now();
-
-    // TODO: maybe set appLimited in recovery based on config
   }
 
   recoveryWindow_ = recoveryWindow_ >
@@ -193,7 +190,6 @@ void BbrCongestionController::onPacketAcked(
   }
 
   bool newRoundTrip = updateRoundTripCounter(ack.largestAckedPacketSentTime);
-  // TODO: I actually don't know why the last one is so special
   bool lastAckedPacketAppLimited =
       ack.ackedPackets.empty() ? false : ack.largestAckedPacketAppLimited;
   if (bandwidthSampler_) {
@@ -269,7 +265,6 @@ void BbrCongestionController::updatePacing() noexcept {
   } else {
     pacingWindow_ = std::max(pacingWindow_, targetPacingWindow);
   }
-  // TODO: slower pacing if we are in STARTUP and loss has happened
   if (state_ == BbrState::Startup) {
     conn_.pacer->setRttFactor(
         conn_.transportSettings.startupRttFactor.first,

quic/congestion_control/QuicCubic.h

@@ -55,7 +55,6 @@ class Cubic : public CongestionController {
    * spreadacrossRtt:   if the pacing bursts should be spread across RTT or all
    *                    close to the beginning of an RTT round
    */
-  // TODO: We haven't experimented with setting ackTrain and tcpFriendly
   explicit Cubic(
       QuicConnectionStateBase& conn,
       uint64_t initCwndBytes = 0,

quic/loss/QuicLossFunctions.cpp

@@ -181,7 +181,6 @@ void markPacketLoss(
           break;
         }
         auto stream = conn.streamManager->getStream(frame.streamId);
-        // TODO: check for retransmittable
         if (!stream) {
           break;
         }

quic/server/QuicServerTransport.cpp

@@ -54,9 +54,6 @@ QuicServerTransport::QuicServerTransport(
   conn_.reset(tempConn.release());
   conn_->observers = observers_;
 
-  // TODO: generate this when we can encode the packet sequence number
-  // correctly.
-  // conn_->nextSequenceNum = folly::Random::secureRandom<PacketNum>();
   setConnectionCallback(&cb);
   registerAllTransportKnobParamHandlers();
 }
@@ -73,8 +70,6 @@ QuicServerTransport::~QuicServerTransport() {
       false);
 }
 
-// TODO: refactor this API so that the factory does not have to create an
-// owning reference.
 QuicServerTransport::Ptr QuicServerTransport::make(
     folly::EventBase* evb,
     std::unique_ptr<folly::AsyncUDPSocket> sock,

quic/server/QuicServerWorker.cpp

@@ -1202,7 +1202,6 @@ void QuicServerWorker::onConnectionUnbound(
     }
   }
 
-  // TODO: verify we are removing the right transport
   sourceAddressMap_.erase(source);
 }
 

quic/server/state/ServerStateMachine.cpp

@@ -813,7 +813,6 @@ void onServerReadDataFromOpen(
     auto packetNum = regularOptional->header.getPacketSequenceNum();
     auto packetNumberSpace = regularOptional->header.getPacketNumberSpace();
 
-    // TODO: enforce constraints on other protection levels.
     auto& regularPacket = *regularOptional;
 
     bool isProtectedPacket = protectionLevel == ProtectionType::ZeroRtt ||
@@ -909,8 +908,6 @@ void onServerReadDataFromOpen(
     bool isNonProbingPacket = false;
     bool handshakeConfirmedThisLoop = false;
 
-    // TODO: possibly drop the packet here, but rolling back state of
-    // what we've already processed is difficult.
     for (auto& quicFrame : regularPacket.frames) {
       switch (quicFrame.type()) {
         case QuicFrame::Type::ReadAckFrame: {
@@ -1404,7 +1401,6 @@ QuicServerConnectionState::createAndAddNewSelfConnId() {
       transportSettings.statelessResetTokenSecret.value(),
       serverAddr.getFullyQualified());
 
-  // TODO Possibly change this mechanism later
   // The default connectionId algo has 36 bits of randomness.
   auto encodedCid = connIdAlgo->encodeConnectionId(*serverConnIdParams);
   size_t encodedTimes = 0;

quic/server/state/ServerStateMachine.h

@@ -148,8 +148,6 @@ struct QuicServerConnectionState : public QuicConnectionStateBase {
     // Create the crypto stream.
     cryptoState = std::make_unique<QuicCryptoState>();
     congestionController = std::make_unique<Cubic>(*this);
-    // TODO: this is wrong, it should be the handshake finish time. But i need
-    // a relatively sane time now to make the timestamps all sane.
     connectionTime = Clock::now();
     supportedVersions = std::vector<QuicVersion>{
         {QuicVersion::MVFST,

quic/state/AckHandlers.cpp

@@ -81,8 +81,6 @@ void processAckFrame(
       break;
     }
 
-    // TODO: only process ACKs from packets which are sent from a greater than
-    // or equal to crypto protection level.
     auto eraseEnd = rPacketIt;
     while (rPacketIt != conn.outstandings.packets.rend()) {
       auto currentPacketNum = rPacketIt->packet.header.getPacketSequenceNum();

quic/state/AckStates.h

@@ -39,8 +39,6 @@ struct AckState {
   // Flag indicating that if we need to send ack immediately. This will be set
   // to true if we got packets with retransmittable data and haven't sent the
   // ack for the first time.
-  // TODO: upgrade this to be bundled like non-retransmittable packets once we
-  // figured out how to do timeout.
   bool needsToSendAckImmediately{false};
   // Count of oustanding packets received with retransmittable data.
   uint8_t numRxPacketsRecvd{0};

quic/state/QuicStateFunctions.cpp

@@ -126,7 +126,6 @@ void updateAckSendStateOnRecvPacket(
         << static_cast<int>(ackState.numNonRxPacketsRecvd)
         << " numRxPacketsRecvd="
         << static_cast<int>(ackState.numRxPacketsRecvd);
-    // TODO: experiment with outOfOrder and ack timer for NonRxPacket too
     conn.pendingEvents.scheduleAckTimeout = false;
     ackState.needsToSendAckImmediately = true;
   }

quic/state/SimpleFrameFunctions.cpp

@@ -243,7 +243,6 @@ bool updateSimpleFrameOnPacketReceived(
       return true;
     }
     case QuicSimpleFrame::Type::KnobFrame: {
-      // TODO it's const so we have to copy it
       const KnobFrame& knobFrame = *frame.asKnobFrame();
       conn.pendingEvents.knobs.emplace_back(
           knobFrame.knobSpace, knobFrame.id, knobFrame.blob->clone());

quic/state/StateData.h

@@ -115,7 +115,6 @@ struct OutstandingsInfo {
   // All PacketEvents of this connection. If a OutstandingPacket doesn't have an
   // associatedEvent or if it's not in this set, there is no need to process its
   // frames upon ack or loss.
-  // TODO: Enforce only AppTraffic packets to be clonable
   folly::F14FastSet<PacketEvent, PacketEventHash> packetEvents;
 
   // Number of outstanding packets not including cloned
@@ -341,11 +340,6 @@ struct CongestionController {
 
   /**
    * Take bytes out of flight without mutating other states of the controller
-   * TODO(yangchi): I'm not sure how long I'd like to keep this API. This is a
-   * temporary workaround the fact that there are packets we will need to take
-   * out of outstandings.packets but not considered loss for congestion
-   * control perspective. In long term, we shouldn't take them out of
-   * outstandings.packets, then we don't have to do this.
    */
   virtual void onRemoveBytesFromInflight(uint64_t) = 0;
   virtual void onPacketSent(const OutstandingPacket& packet) = 0;
@@ -848,8 +842,6 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction {
   // Flag indicating whether the socket is currently waiting for the app to
   // write data. All new sockets start off in this state where they wait for the
   // application to pump data to the socket.
-  // TODO: Merge this flag with the existing appLimited flag that exists on each
-  // Congestion Controller.
   bool waitingForAppData{true};
 
   // Monotonically increasing counter that is incremented each time there is a