Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TIMEOUT CONNECTION_CLOSE error #3126

Closed
rpaulo opened this issue Oct 21, 2019 · 9 comments
Closed

TIMEOUT CONNECTION_CLOSE error #3126

rpaulo opened this issue Oct 21, 2019 · 9 comments
Labels
-transport design has-consensus

Comments

@rpaulo
Copy link
Contributor

@rpaulo rpaulo commented Oct 21, 2019

Perhaps we should have a TIMEOUT error code? Connection timeouts are extremely common. It would help implementations detect timeout conditions without having to parse the connection_close error string. One could argue that a timeout isn't an error, but we already have SERVER_BUSY.

@mikkelfj
Copy link
Contributor

@mikkelfj mikkelfj commented Oct 21, 2019

If so, it should probably distinguish between idle timeout and handshake timeout.

@ianswett
Copy link
Contributor

@ianswett ianswett commented Oct 21, 2019

I think the transport doc currently suggests using NO_ERROR. There are no other suggested uses of NO_ERROR, so this seems like it is functionally equivalent to a TIMEOUT error code?

After receiving a CONNECTION_CLOSE frame, endpoints enter the draining state. An endpoint that receives a CONNECTION_CLOSE frame MAY send a single packet containing a CONNECTION_CLOSE frame before entering the draining state, using a CONNECTION_CLOSE frame and a NO_ERROR code if appropriate. An endpoint MUST NOT send further packets, which could result in a constant exchange of CONNECTION_CLOSE frames until the closing period on either peer ended.

@rpaulo
Copy link
Contributor Author

@rpaulo rpaulo commented Oct 21, 2019

@ianswett When I read that, it doesn't really sound like it's describing a timeout condition, just that one side is about to enter the draining state and it may send a CONNECTION_CLOSE packet to increase the chances the other side also moves to the draining state.

NO_ERROR doesn't seem that useful to me.

@marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Oct 21, 2019

If my understanding of the spec is correct, you're not supposed to send a CONNECTION_CLOSE when you detect a timeout, since you don't expect it to be received anyway.

@mnot mnot added this to Triage in Late Stage Processing Oct 21, 2019
@rpaulo
Copy link
Contributor Author

@rpaulo rpaulo commented Oct 21, 2019

I was thinking about the case where the idle timeout is long and a peer would be stuck retransmitting data until it hits the idle timeout. Right now, it can send NO_ERROR, but it just seems like having a specific error code for timeouts would help. I agree that there's no need to send anything after we reach the idle timeout.

@ianswett
Copy link
Contributor

@ianswett ianswett commented Oct 22, 2019

I prefer more error codes over fewer, especially for cases like this that may be quite common.

Even if we use NO_ERROR, I think we should specify what the error code to send is in the case you're mentioning.

@martinthomson
Copy link
Member

@martinthomson martinthomson commented Oct 22, 2019

Why would this condition need to be signaled?

@rpaulo
Copy link
Contributor Author

@rpaulo rpaulo commented Oct 22, 2019

@martinthomson I think it could help identify problems when the link is asymmetrically broken and you don't have access to the state of the server.

@larseggert larseggert added the design label Feb 4, 2020
@project-bot project-bot bot moved this from Triage to Design Issues in Late Stage Processing Feb 4, 2020
@larseggert
Copy link
Member

@larseggert larseggert commented Feb 5, 2020

Discussed in ZRH. Proposed resolution is to close with no action.

@larseggert larseggert added the proposal-ready label Feb 5, 2020
@project-bot project-bot bot moved this from Design Issues to Consensus Emerging in Late Stage Processing Feb 5, 2020
@LPardue LPardue moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Feb 19, 2020
@LPardue LPardue removed the proposal-ready label Feb 19, 2020
@LPardue LPardue added the call-issued label Feb 26, 2020
@LPardue LPardue added has-consensus and removed call-issued labels Mar 4, 2020
@project-bot project-bot bot moved this from Consensus Call issued to Consensus Declared in Late Stage Processing Mar 4, 2020
Late Stage Processing automation moved this from Consensus Declared to Text Incorporated Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design has-consensus
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

7 participants