Disable the HttpSM half open logic if the underlying transport is TLS#4213
Merged
shinrich merged 1 commit intoapache:masterfrom Sep 10, 2018
Merged
Conversation
| bool | ||
| allow_half_open() | ||
| { | ||
| bool retval = false; |
Member
There was a problem hiding this comment.
return dynamic_cast<SSLNetVConnection *>(client_vc) == nullptr;?
| return current_reader->t_state.txn_conf->allow_half_open > 0; | ||
| } else { | ||
| // Check with the session to make sure the underlying transport allows the half open scenario | ||
| return dynamic_cast<Http1ClientSession *>(parent)->allow_half_open(); |
Member
There was a problem hiding this comment.
Why not static_cast, if you are assuming the result is valid?
shinrich
force-pushed
the
turn-off-half-open-for-tls
branch
from
22a934b to
89d891e
Compare
Member
Author
|
Pushed a new version addressing @SolidWallOfCode's comments. |
SolidWallOfCode
approved these changes
Sep 7, 2018
Contributor
|
Cherry picked to 8.0.0 |
Contributor
|
@shinrich -- Do we need to prepare a back-port PR for 7.1.x? Can you do this, or do you want me to do it using the custom patch that you provided to us earlier (with the same change recommended by @bryancall above)? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jeremy and @pbchou reported having a TLS client send a client-notify and FIN but ATS would continue sending data back. This sounded like a bad side effect of the half-open logic in HttpSM. If the underlying protocol is just TCP it could be legitimate to sending back traffic after the client sends a FIN. The client may still be listening. But if the underlying protocol is TLS, this half-open scenario makes no sense.
For HTTP2, we always disable the HttpSM half open logic. This PR extends that logic for HTTP1 if the client_vc is a SSLNetVConn.
Jermey did some initial testing with a patch against 7.1.x and this solves the problem. The patch is a little different for master since that already includes the setting to turn of the half open logic entirely.