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

API 5.2 Connection timeout #109

Closed
csperkins opened this Issue Feb 27, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@csperkins
Contributor

csperkins commented Feb 27, 2018

This API says:

* Suggest a timeout to the Remote Endpoint:
  This boolean property specifies whether an application considers it
  useful to propose a timeout until the connection is assumed to be lost.
  This property applies to Connections and Connection Groups. This is not a
  strict requirement. The default is to have this option.

Need to clarify if this is a timeout for connection establishment (i.e., how long should the endpoint wait before declaring that Initiate() failed), or it it's a timeout for data transfer after a connection has been established?

@mwelzl

This comment has been minimized.

Contributor

mwelzl commented Feb 27, 2018

Before you do Initiate, it's the timeout for establishment. After the connection has been established, it's the timeout for data transfer. It was my idea that one notion of a timeout is enough - you will never need both in parallel for the same Connection or Preconnection.

@tfpauly

This comment has been minimized.

Contributor

tfpauly commented Feb 27, 2018

Those seem like two very different timeouts, and ones for which the application may have very different tolerances.

Personally, I’ve been averse to giving timeout knobs in general, since choosing good timeouts is hard, and a knob makes developers think they need to turn it. Most of the time, no timeout is best (talk to Stuart). Beyond that, any establishment or receive timeout can just as easily be implemented in the application as they wait for their Ready or Receive Complete event. This knob is a convenience to re-arm a timer, not really a transport service itself.

@britram

This comment has been minimized.

Contributor

britram commented Feb 27, 2018

Move to Appendix A, then?

@mwelzl

This comment has been minimized.

Contributor

mwelzl commented Feb 27, 2018

I agree that these timeout values may be quite different; maybe merging them into only one parameter wasn't a good idea.

About this:

Personally, I’ve been averse to giving timeout knobs in general, since choosing good timeouts is hard, and a knob makes developers think they need to turn it. Most of the time, no timeout is best (talk to Stuart). Beyond that, any establishment or receive timeout can just as easily be implemented in the application as they wait for their Ready or Receive Complete event. This knob is a convenience to re-arm a timer, not really a transport service itself.

Let me try to understand this: do you mean to say that the TCP User Timeout should simply be set to a huge number, large enough to basically never fire, and then it's up to the application to decide when to give up?

I'd have to think about this... note that this comes via the TCP RFCs => RFC 8303 => minset path, it's not my invention.

@tfpauly

This comment has been minimized.

Contributor

tfpauly commented Feb 27, 2018

@mwelzl I'm not arguing against TCP having a timeout option, but the discussion of this option in the API—especially as a one top-level, cross protocol, all-encompassing timeout—was going in a direction of adding timeouts where we wouldn't otherwise have them. If we had a timeout on overall connection establishment, from Initiate() to Ready(), then that's trying to set a timeout not just on TCP, but on DNS, racing, proxies, TLS, and who knows what else. I tend to see the timeout value as a specific protocol option if anything; sure, you can set a timer on any protocol you want, but is that really a benefit to the application? If a protocol behaves differently than TCP in the future with regards to how it acknowledges data, should setting the TCP User Timeout for how long data can be un-acked apply to this future transport, or are we starting to ossify it?

@mwelzl

This comment has been minimized.

Contributor

mwelzl commented Feb 27, 2018

@tfpauly : first, I didn't think about the timeout being so all-encompassing (DNS, racing, etc.) - but you're right, the way we have it, it becomes like that, and this sure sounds like a horrible idea. So I agree that this should only be a specific protocol option if anything.

Regarding "if anything", I understand your arguments and tend to agree with them, too. However, I'm just a little careful about things coming from minset - I think that we must have sound arguments to remove them, of the style "functionality XY covers this, and it can be dealt with inside the transport system because ...". After all, the UTO exists for a reason...

Noticing your thumb up for @britram 's suggestion to move this to appendix A, I think we're all in agreement on this approach for now.

@tfpauly

This comment has been minimized.

Contributor

tfpauly commented Feb 27, 2018

Yeah, totally fine with this being in Appendix A for now. Definitely merits more discussion.

@britram britram self-assigned this Feb 27, 2018

britram added a commit that referenced this issue Feb 27, 2018

@britram

This comment has been minimized.

Contributor

britram commented Feb 27, 2018

see #117

@britram britram modified the milestones: London, post-London Feb 27, 2018

@britram britram added the discuss label Feb 27, 2018

@britram britram removed their assignment Feb 27, 2018

@britram

This comment has been minimized.

Contributor

britram commented Feb 27, 2018

(mutating this issue to be the place to discuss these options)

britram added a commit that referenced this issue Feb 27, 2018

Merge pull request #117 from taps-api/we-dont-believe-in-timeouts-yet
move timeout suggestion to appendix A (see #109)

@britram britram closed this Mar 21, 2018

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