masaori335 commented on a change in pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#discussion_r698220306
##########
File path: proxy/http2/Http2Stream.cc
##########
@@ -273,103 +273,111 @@ Http2Stream::send_request(Http2ConnectionState &cstate)
bool
Http2Stream::change_state(uint8_t type, uint8_t flags)
{
- switch (_state) {
- case Http2StreamState::HTTP2_STREAM_STATE_IDLE:
- if (type == HTTP2_FRAME_TYPE_HEADERS) {
- if (recv_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
- } else if (send_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL;
+ Http2StreamState original_state = _state;
+ bool retval = true;
+ if (type == HTTP2_FRAME_TYPE_RST_STREAM) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
+ } else {
+ switch (_state) {
+ case Http2StreamState::HTTP2_STREAM_STATE_IDLE:
+ if (type == HTTP2_FRAME_TYPE_HEADERS) {
+ if (recv_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
+ } else if (send_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL;
+ } else {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_OPEN;
+ }
+ } else if (type == HTTP2_FRAME_TYPE_CONTINUATION) {
+ if (recv_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
+ } else if (send_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL;
+ } else {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_OPEN;
+ }
+ } else if (type == HTTP2_FRAME_TYPE_PUSH_PROMISE) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_RESERVED_LOCAL;
} else {
- _state = Http2StreamState::HTTP2_STREAM_STATE_OPEN;
+ retval = false;
}
- } else if (type == HTTP2_FRAME_TYPE_CONTINUATION) {
- if (recv_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
- } else if (send_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL;
+ break;
+
+ case Http2StreamState::HTTP2_STREAM_STATE_OPEN:
+ if (type == HTTP2_FRAME_TYPE_HEADERS || type == HTTP2_FRAME_TYPE_DATA) {
+ if (recv_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
+ } else if (send_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL;
+ }
+ // Otherwise Do not change state
} else {
- _state = Http2StreamState::HTTP2_STREAM_STATE_OPEN;
+ // A stream in the "open" state may be used by both peers to send
frames of any type.
}
- } else if (type == HTTP2_FRAME_TYPE_PUSH_PROMISE) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_RESERVED_LOCAL;
- } else {
- return false;
- }
- break;
+ break;
- case Http2StreamState::HTTP2_STREAM_STATE_OPEN:
- if (type == HTTP2_FRAME_TYPE_RST_STREAM) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
- } else if (type == HTTP2_FRAME_TYPE_HEADERS || type ==
HTTP2_FRAME_TYPE_DATA) {
- if (recv_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
- } else if (send_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL;
+ case Http2StreamState::HTTP2_STREAM_STATE_RESERVED_LOCAL:
+ if (type == HTTP2_FRAME_TYPE_HEADERS) {
+ if (flags & HTTP2_FLAGS_HEADERS_END_HEADERS) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
+ }
+ } else if (type == HTTP2_FRAME_TYPE_CONTINUATION) {
+ if (flags & HTTP2_FLAGS_CONTINUATION_END_HEADERS) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
+ }
} else {
- // Do not change state
+ retval = false;
}
- } else {
- // A stream in the "open" state may be used by both peers to send frames
of any type.
- return true;
- }
- break;
+ break;
- case Http2StreamState::HTTP2_STREAM_STATE_RESERVED_LOCAL:
- if (type == HTTP2_FRAME_TYPE_HEADERS) {
- if (flags & HTTP2_FLAGS_HEADERS_END_HEADERS) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
- }
- } else if (type == HTTP2_FRAME_TYPE_CONTINUATION) {
- if (flags & HTTP2_FLAGS_CONTINUATION_END_HEADERS) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE;
- }
- } else {
- return false;
- }
- break;
+ case Http2StreamState::HTTP2_STREAM_STATE_RESERVED_REMOTE:
+ // Currently ATS supports only HTTP/2 server features
+ retval = false;
+ break;
- case Http2StreamState::HTTP2_STREAM_STATE_RESERVED_REMOTE:
- // Currently ATS supports only HTTP/2 server features
- return false;
+ case Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL:
+ if (recv_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
+ } else {
+ // Error, set state closed
+ _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
+ retval = false;
+ }
+ break;
- case Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL:
- if (type == HTTP2_FRAME_TYPE_RST_STREAM || recv_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
- } else {
- // Error, set state closed
- _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
- return false;
- }
- break;
+ case Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE:
+ if (send_end_stream) {
+ _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
+ } else if (type == HTTP2_FRAME_TYPE_HEADERS) { // w/o END_STREAM flag
+ // No state change here. Expect a following DATA frame with END_STREAM
flag.
+ } else if (type == HTTP2_FRAME_TYPE_CONTINUATION) { // w/o END_STREAM
flag
+ // No state change here. Expect a following DATA frame with END_STREAM
flag.
+ } else {
+ // Error, set state closed
+ _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
+ retval = false;
+ }
+ break;
- case Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_REMOTE:
- if (type == HTTP2_FRAME_TYPE_RST_STREAM || send_end_stream) {
- _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
- } else if (type == HTTP2_FRAME_TYPE_HEADERS) { // w/o END_STREAM flag
- // No state change here. Expect a following DATA frame with END_STREAM
flag.
- return true;
- } else if (type == HTTP2_FRAME_TYPE_CONTINUATION) { // w/o END_STREAM flag
- // No state change here. Expect a following DATA frame with END_STREAM
flag.
- return true;
- } else {
- // Error, set state closed
- _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
- return false;
+ case Http2StreamState::HTTP2_STREAM_STATE_CLOSED:
+ // No state changing
+ break;
+ default:
+ retval = false;
+ break;
}
- break;
-
- case Http2StreamState::HTTP2_STREAM_STATE_CLOSED:
- // No state changing
- return true;
+ }
- default:
- return false;
+ // Did we just transition to closed or remote closed? Decrement stream count
+ if (_state == Http2StreamState::HTTP2_STREAM_STATE_CLOSED && original_state
!= Http2StreamState::HTTP2_STREAM_STATE_CLOSED) {
+ Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession
*>(this->_proxy_ssn);
+ SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex,
this_ethread());
+ h2_proxy_ssn->connection_state.decrement_stream_count(this->_id);
Review comment:
I can understand we can check state changes here, but I'm not a big fan
of giving another task to the `Http2Stream::change_state()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
