Skip to content

Conversation

@zizhong
Copy link
Member

@zizhong zizhong commented Mar 22, 2018

This is a workaround for issue #3288.

The root cause of this issue is when ATS gets RST in the state of half-close, the read side is closed and the RST is ignored.
#947 is trying to bring up the RST to HttpSM as a VIO error event, but the SM doesn't expect an event on a disabled read, so the SM ends up crashing.

This PR is to give a workaround with little risk until we figure out what the proper way to fix #3288.

@bryancall @zwoop @oknet @scw00 any ideas would be much appreciated.

@scw00 scw00 self-requested a review March 22, 2018 03:41
@scw00 scw00 self-assigned this Mar 22, 2018
@scw00 scw00 added the HTTP label Mar 22, 2018
@scw00 scw00 added this to the 8.0.0 milestone Mar 22, 2018
@scw00 scw00 added the Network label Mar 22, 2018
@scw00 scw00 assigned zizhong and unassigned scw00 Mar 22, 2018
@zizhong zizhong force-pushed the configurable_half_close branch from d638532 to 007bf1b Compare March 22, 2018 22:26
@zizhong zizhong force-pushed the configurable_half_close branch from 007bf1b to 92e660a Compare March 23, 2018 00:46
Copy link
Member

@scw00 scw00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ! thanks

@scw00 scw00 requested review from bryancall and zwoop March 26, 2018 08:45
@scw00
Copy link
Member

scw00 commented Mar 26, 2018

Hi @zwoop @bryancall
This pr make half-closed configurable, but I'm not sure whether we should launch this pr . Could you please review again ?


Turn on or off support for connection half open for client side. Default is on, so
after client sends FIN, the connection is still there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please inform the end user that it is dangerous to turn it off. It can only be turned off in special scenario.

@bryancall
Copy link
Contributor

We plan to talk about half open at the summit this Spring. Please hold off merging this till after.

@shinrich
Copy link
Member

shinrich commented May 2, 2018

The implementation looks fine to me. Should probably rename it to allow_half_closed if we decide to go with this. Probably my fault. I added allow_half_open method to the ProxyClient* classes. In the general community it seems that half open refers to embryonic connections (got the SYN but not the SYN-ACK). And half closed refers to this situation. Most of the code refers to half_close_flag's, so I just had a brain bubble when naming my method.

@shinrich
Copy link
Member

In the discussion at the Euro Tour, the consensus was to add this flag and during the 8.0 time frame add metrics to figure out how often this really happens for various scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reopen TS-4796

5 participants