Reduce possibility for H2 client count mismatch#8202
Reduce possibility for H2 client count mismatch#8202shinrich wants to merge 2 commits intoapache:masterfrom
Conversation
|
[approve ci clang-analyzer] |
|
[approve ci autest] |
|
@masaori335 is going to look at it. |
| 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); |
There was a problem hiding this 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().
|
[approve ci] |
1 similar comment
|
[approve ci] |
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
Decrementing stream count before removing a stream from stream_list sounds dangerous. That could allow keeping a lot of stream instances in the list. I think we should not do it separately at different places.
If I understand the point of this PR correctly, it's decrementing the count as soon as possible when it gets obvious to close a stream. I agree that it would reduce possibility for the count mismatch a little, but I'm not sure if the change helps a lot. I'd try to delete a stream asap if the delay matters.
And it seems like the root cause of the mismatch is a bug in ATS's client code, according to the last comment on #8200. I suggest closing this PR without landing.
|
[approve ci rocky] |
|
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community. |
6 participants
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