From 003f012cb7f5224770eaca985cc96b53da496834 Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Tue, 20 Jul 2021 10:26:47 -0700 Subject: [PATCH] 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 | 2 -- quic/api/IoBufQuicBatch.cpp | 6 +----- quic/api/QuicPacketScheduler.cpp | 3 +-- quic/api/QuicPacketScheduler.h | 3 --- quic/api/QuicTransportBase.cpp | 8 +------- quic/api/QuicTransportFunctions.cpp | 5 ----- quic/api/QuicTransportFunctions.h | 1 - quic/client/QuicClientTransport.cpp | 15 --------------- quic/client/state/ClientStateMachine.cpp | 2 -- quic/client/state/ClientStateMachine.h | 2 -- quic/codec/Decode.cpp | 2 -- quic/codec/QuicPacketBuilder.h | 1 - quic/codec/QuicReadCodec.cpp | 1 - quic/codec/QuicWriteCodec.cpp | 2 -- quic/codec/Types.h | 7 ------- quic/congestion_control/Bbr.cpp | 5 ----- quic/congestion_control/QuicCubic.h | 1 - quic/loss/QuicLossFunctions.cpp | 1 - quic/server/QuicServerTransport.cpp | 5 ----- quic/server/QuicServerWorker.cpp | 1 - quic/server/state/ServerStateMachine.cpp | 4 ---- quic/server/state/ServerStateMachine.h | 2 -- quic/state/AckHandlers.cpp | 2 -- quic/state/AckStates.h | 2 -- quic/state/QuicStateFunctions.cpp | 1 - quic/state/SimpleFrameFunctions.cpp | 1 - quic/state/StateData.h | 8 -------- 27 files changed, 3 insertions(+), 90 deletions(-) diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 81bd4b01b..6f280736d 100644 --- a/quic/QuicConstants.h +++ b/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 diff --git a/quic/api/IoBufQuicBatch.cpp b/quic/api/IoBufQuicBatch.cpp index 430da3f36..165ba75ed 100644 --- a/quic/api/IoBufQuicBatch.cpp +++ b/quic/api/IoBufQuicBatch.cpp @@ -97,9 +97,6 @@ bool IOBufQuicBatch::flushInternal() { folly::Optional 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 } diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 8bdfc1715..fec882def 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/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; diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index 14d5ea898..4c9ca144a 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/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, diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index c849f0878..9a29abe53 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/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); } diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 9fcd934f4..7bd1a00f9 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/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 diff --git a/quic/api/QuicTransportFunctions.h b/quic/api/QuicTransportFunctions.h index 50b24a538..7fbdb1fd9 100644 --- a/quic/api/QuicTransportFunctions.h +++ b/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); diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 348cf1ac3..6486014a8 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/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(); 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. diff --git a/quic/client/state/ClientStateMachine.cpp b/quic/client/state/ClientStateMachine.cpp index da248f312..9bb882cfd 100644 --- a/quic/client/state/ClientStateMachine.cpp +++ b/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; diff --git a/quic/client/state/ClientStateMachine.h b/quic/client/state/ClientStateMachine.h index 27819fd99..1b0739552 100644 --- a/quic/client/state/ClientStateMachine.h +++ b/quic/client/state/ClientStateMachine.h @@ -77,8 +77,6 @@ struct QuicClientConnectionState : public QuicConnectionStateBase { handshakeFactory(std::move(handshakeFactoryIn)) { cryptoState = std::make_unique(); congestionController = std::make_unique(*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); diff --git a/quic/codec/Decode.cpp b/quic/codec/Decode.cpp index ec5eaa474..71ce7b1b6 100644 --- a/quic/codec/Decode.cpp +++ b/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); diff --git a/quic/codec/QuicPacketBuilder.h b/quic/codec/QuicPacketBuilder.h index af0293f6b..c522a9f36 100644 --- a/quic/codec/QuicPacketBuilder.h +++ b/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; diff --git a/quic/codec/QuicReadCodec.cpp b/quic/codec/QuicReadCodec.cpp index 51844c536..6dde47c2e 100644 --- a/quic/codec/QuicReadCodec.cpp +++ b/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 largestReceivedPacketNum; switch (longHeaderTypeToProtectionType(type)) { diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index 79690146a..421836580 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -262,8 +262,6 @@ folly::Optional 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 diff --git a/quic/codec/Types.h b/quic/codec/Types.h index 222c90739..42a6cce8a 100644 --- a/quic/codec/Types.h +++ b/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; diff --git a/quic/congestion_control/Bbr.cpp b/quic/congestion_control/Bbr.cpp index fcee23e52..97a955d7b 100644 --- a/quic/congestion_control/Bbr.cpp +++ b/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, diff --git a/quic/congestion_control/QuicCubic.h b/quic/congestion_control/QuicCubic.h index 6d876e354..31dc789ae 100644 --- a/quic/congestion_control/QuicCubic.h +++ b/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, diff --git a/quic/loss/QuicLossFunctions.cpp b/quic/loss/QuicLossFunctions.cpp index 7ae41b8bf..9a1da26bb 100644 --- a/quic/loss/QuicLossFunctions.cpp +++ b/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; } diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 011084520..730b9eb56 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/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(); 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 sock, diff --git a/quic/server/QuicServerWorker.cpp b/quic/server/QuicServerWorker.cpp index dca080fa2..3ba7d3b3d 100644 --- a/quic/server/QuicServerWorker.cpp +++ b/quic/server/QuicServerWorker.cpp @@ -1202,7 +1202,6 @@ void QuicServerWorker::onConnectionUnbound( } } - // TODO: verify we are removing the right transport sourceAddressMap_.erase(source); } diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 01faeafef..26ed1ff14 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/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; diff --git a/quic/server/state/ServerStateMachine.h b/quic/server/state/ServerStateMachine.h index 6274589c1..623f53f53 100644 --- a/quic/server/state/ServerStateMachine.h +++ b/quic/server/state/ServerStateMachine.h @@ -148,8 +148,6 @@ struct QuicServerConnectionState : public QuicConnectionStateBase { // Create the crypto stream. cryptoState = std::make_unique(); congestionController = std::make_unique(*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::MVFST, diff --git a/quic/state/AckHandlers.cpp b/quic/state/AckHandlers.cpp index fca17fb7f..8412df854 100644 --- a/quic/state/AckHandlers.cpp +++ b/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(); diff --git a/quic/state/AckStates.h b/quic/state/AckStates.h index 2113c8323..e62d0ec89 100644 --- a/quic/state/AckStates.h +++ b/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}; diff --git a/quic/state/QuicStateFunctions.cpp b/quic/state/QuicStateFunctions.cpp index 3c5794565..65de8454b 100644 --- a/quic/state/QuicStateFunctions.cpp +++ b/quic/state/QuicStateFunctions.cpp @@ -126,7 +126,6 @@ void updateAckSendStateOnRecvPacket( << static_cast(ackState.numNonRxPacketsRecvd) << " numRxPacketsRecvd=" << static_cast(ackState.numRxPacketsRecvd); - // TODO: experiment with outOfOrder and ack timer for NonRxPacket too conn.pendingEvents.scheduleAckTimeout = false; ackState.needsToSendAckImmediately = true; } diff --git a/quic/state/SimpleFrameFunctions.cpp b/quic/state/SimpleFrameFunctions.cpp index eb82802d5..5e42dd3cb 100644 --- a/quic/state/SimpleFrameFunctions.cpp +++ b/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()); diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 6d9bd32a0..fc708a86e 100644 --- a/quic/state/StateData.h +++ b/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 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