Simplify HttpDecoder state transitions. Both before and after this CL, non-buffered frames go through the following states: STATE_READING_FRAME_LENGTH STATE_READING_FRAME_TYPE STATE_READING_FRAME_PAYLOAD STATE_FINISH_PARSING They skip STATE_READING_FRAME_PAYLOAD if the payload is empty. Before this CL, buffered frames normally skip STATE_FINISH_PARSING, except they skip STATE_READING_FRAME_PAYLOAD if payload is empty just like non-buffered states, and therefore they have to go through STATE_FINISH_PARSING. ReadFramePayload() corresponds to STATE_READING_FRAME_PAYLOAD and FinishParsing() corresponds to STATE_FINISH_PARSING, and states are only changed in methods that correspond to states, except for the helper BufferOrParsePayload() which does not have a corresponding state but can still change the state, which its caller must detect. After this CL, the states STATE_READING_FRAME_PAYLOAD and STATE_FINISH_PARSING only process non-buffered frames, and buffered frames go through a dedicated new state STATE_BUFFER_OR_PARSE_PAYLOAD. The state transition path forks in ReadFrameLength(). BufferOrParsePayload() gets promoted from a random helper called from other methods to a method corresponding to a state, called from ProcessInput() which drives the state machine. State transitions are now cleaner. PiperOrigin-RevId: 399487182
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index 2265b77..02aa123 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -100,8 +100,11 @@ QuicDataReader reader(data, len); bool continue_processing = true; - while (continue_processing && - (reader.BytesRemaining() != 0 || state_ == STATE_FINISH_PARSING)) { + // BufferOrParsePayload() and FinishParsing() may need to be called even if + // there is no more data so that they can finish processing the current frame. + while (continue_processing && (reader.BytesRemaining() != 0 || + state_ == STATE_BUFFER_OR_PARSE_PAYLOAD || + state_ == STATE_FINISH_PARSING)) { // |continue_processing| must have been set to false upon error. QUICHE_DCHECK_EQ(QUIC_NO_ERROR, error_); QUICHE_DCHECK_NE(STATE_ERROR, state_); @@ -113,11 +116,14 @@ case STATE_READING_FRAME_LENGTH: continue_processing = ReadFrameLength(&reader); break; + case STATE_BUFFER_OR_PARSE_PAYLOAD: + continue_processing = BufferOrParsePayload(&reader); + break; case STATE_READING_FRAME_PAYLOAD: continue_processing = ReadFramePayload(&reader); break; case STATE_FINISH_PARSING: - continue_processing = FinishParsing(&reader); + continue_processing = FinishParsing(); break; case STATE_PARSING_NO_LONGER_POSSIBLE: continue_processing = false; @@ -284,6 +290,12 @@ } remaining_frame_length_ = current_frame_length_; + + if (IsFrameBuffered()) { + state_ = STATE_BUFFER_OR_PARSE_PAYLOAD; + return continue_processing; + } + state_ = (remaining_frame_length_ == 0) ? STATE_FINISH_PARSING : STATE_READING_FRAME_PAYLOAD; return continue_processing; @@ -308,6 +320,7 @@ } bool HttpDecoder::ReadFramePayload(QuicDataReader* reader) { + QUICHE_DCHECK(!IsFrameBuffered()); QUICHE_DCHECK_NE(0u, reader->BytesRemaining()); QUICHE_DCHECK_NE(0u, remaining_frame_length_); @@ -341,7 +354,7 @@ break; } case static_cast<uint64_t>(HttpFrameType::SETTINGS): { - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): { @@ -349,19 +362,19 @@ break; } case static_cast<uint64_t>(HttpFrameType::GOAWAY): { - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID): { - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM): { - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } default: { @@ -370,26 +383,15 @@ } } - if (IsFrameBuffered()) { - if (state_ != STATE_READING_FRAME_PAYLOAD) { - // BufferOrParsePayload() has advanced |state_|. - // TODO(bnc): Simplify state transitions. - QUICHE_DCHECK_EQ(STATE_READING_FRAME_TYPE, state_); - QUICHE_DCHECK_EQ(0u, remaining_frame_length_); - } - } else { - QUICHE_DCHECK(state_ == STATE_READING_FRAME_PAYLOAD); - } - - // BufferOrParsePayload() may have advanced |state_|. - if (state_ == STATE_READING_FRAME_PAYLOAD && remaining_frame_length_ == 0) { + if (remaining_frame_length_ == 0) { state_ = STATE_FINISH_PARSING; } return continue_processing; } -bool HttpDecoder::FinishParsing(QuicDataReader* reader) { +bool HttpDecoder::FinishParsing() { + QUICHE_DCHECK(!IsFrameBuffered()); QUICHE_DCHECK_EQ(0u, remaining_frame_length_); bool continue_processing = true; @@ -408,9 +410,7 @@ break; } case static_cast<uint64_t>(HttpFrameType::SETTINGS): { - // If frame payload is not empty, FinishParsing() is skipped. - QUICHE_DCHECK_EQ(0u, current_frame_length_); - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): { @@ -418,37 +418,33 @@ break; } case static_cast<uint64_t>(HttpFrameType::GOAWAY): { - // If frame payload is not empty, FinishParsing() is skipped. - QUICHE_DCHECK_EQ(0u, current_frame_length_); - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID): { - // If frame payload is not empty, FinishParsing() is skipped. - QUICHE_DCHECK_EQ(0u, current_frame_length_); - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM): { - // If frame payload is not empty, FinishParsing() is skipped. - QUICHE_DCHECK_EQ(0u, current_frame_length_); - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { - // If frame payload is not empty, FinishParsing() is skipped. - QUICHE_DCHECK_EQ(0u, current_frame_length_); - continue_processing = BufferOrParsePayload(reader); + QUICHE_NOTREACHED(); break; } default: continue_processing = visitor_->OnUnknownFrameEnd(); } + ResetForNextFrame(); + return continue_processing; +} + +void HttpDecoder::ResetForNextFrame() { current_length_field_length_ = 0; current_type_field_length_ = 0; state_ = STATE_READING_FRAME_TYPE; - return continue_processing; } bool HttpDecoder::HandleUnknownFramePayload(QuicDataReader* reader) { @@ -467,40 +463,39 @@ QUICHE_DCHECK_EQ(current_frame_length_, buffer_.size() + remaining_frame_length_); - bool continue_processing = true; - if (buffer_.empty() && reader->BytesRemaining() >= current_frame_length_) { // |*reader| contains entire payload, which might be empty. remaining_frame_length_ = 0; QuicDataReader current_payload_reader(reader->PeekRemainingPayload().data(), current_frame_length_); - continue_processing = ParseEntirePayload(¤t_payload_reader); + bool continue_processing = ParseEntirePayload(¤t_payload_reader); + reader->Seek(current_frame_length_); - } else { - // Buffer as much of the payload as |*reader| contains. - QuicByteCount bytes_to_read = std::min<QuicByteCount>( - remaining_frame_length_, reader->BytesRemaining()); - absl::StrAppend(&buffer_, reader->PeekRemainingPayload().substr( - /* pos = */ 0, bytes_to_read)); - reader->Seek(bytes_to_read); - remaining_frame_length_ -= bytes_to_read; - - QUICHE_DCHECK_EQ(current_frame_length_, - buffer_.size() + remaining_frame_length_); - - if (remaining_frame_length_ > 0) { - QUICHE_DCHECK(reader->IsDoneReading()); - return true; - } - - QuicDataReader buffer_reader(buffer_); - continue_processing = ParseEntirePayload(&buffer_reader); - buffer_.clear(); + ResetForNextFrame(); + return continue_processing; } - current_length_field_length_ = 0; - current_type_field_length_ = 0; - state_ = STATE_READING_FRAME_TYPE; + // Buffer as much of the payload as |*reader| contains. + QuicByteCount bytes_to_read = std::min<QuicByteCount>( + remaining_frame_length_, reader->BytesRemaining()); + absl::StrAppend(&buffer_, reader->PeekRemainingPayload().substr( + /* pos = */ 0, bytes_to_read)); + reader->Seek(bytes_to_read); + remaining_frame_length_ -= bytes_to_read; + + QUICHE_DCHECK_EQ(current_frame_length_, + buffer_.size() + remaining_frame_length_); + + if (remaining_frame_length_ > 0) { + QUICHE_DCHECK(reader->IsDoneReading()); + return false; + } + + QuicDataReader buffer_reader(buffer_); + bool continue_processing = ParseEntirePayload(&buffer_reader); + buffer_.clear(); + + ResetForNextFrame(); return continue_processing; }
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h index 002ff68..118fbcf 100644 --- a/quic/core/http/http_decoder.h +++ b/quic/core/http/http_decoder.h
@@ -155,8 +155,14 @@ enum HttpDecoderState { STATE_READING_FRAME_LENGTH, STATE_READING_FRAME_TYPE, + + // States used for buffered frame types + STATE_BUFFER_OR_PARSE_PAYLOAD, + + // States used for non-buffered frame types STATE_READING_FRAME_PAYLOAD, STATE_FINISH_PARSING, + STATE_PARSING_NO_LONGER_POSSIBLE, STATE_ERROR }; @@ -188,7 +194,10 @@ // empty, and it calls BufferOrParsePayload(). For other frame types, this // method directly calls visitor methods to signal that frame had been // received completely. Returns whether processing should continue. - bool FinishParsing(QuicDataReader* reader); + bool FinishParsing(); + + // Reset internal fields to prepare for reading next frame. + void ResetForNextFrame(); // Read payload of unknown frame from |reader| and call // Visitor::OnUnknownFramePayload(). Returns true decoding should continue,