You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/08/03 20:10:59 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #8202: Reduce possibility for H2 client count mismatch
shinrich opened a new pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202
Details in issue #8200. The code change pulls up the decrementing logic for the client stream count, so we can be more likely to be consistent with the H2 peers counting.
This closes #8200
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#issuecomment-893456274
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#issuecomment-895613006
@masaori335 is going to look at it.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#issuecomment-1045224748
[approve ci rocky]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#issuecomment-944415399
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#issuecomment-892980804
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8202: Reduce possibility for H2 client count mismatch
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8202:
URL: https://github.com/apache/trafficserver/pull/8202#issuecomment-892980804
[approve ci clang-analyzer]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org