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

[BUG] If deadline argument to WriteControl is deadline.IsZero() then 1000 hours (~41 days) are used as unexpected and undocumented fallback #895

Closed
1 task done
JannikGM opened this issue Feb 13, 2024 · 2 comments · Fixed by #898
Labels

Comments

@JannikGM
Copy link

JannikGM commented Feb 13, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

If the deadline argument is deadline.IsZero() then 1000 hours are used instead:

websocket/conn.go

Lines 448 to 456 in d08ee1a

d := 1000 * time.Hour
if !deadline.IsZero() {
d = time.Until(deadline)
if d < 0 {
return errWriteTimeout
}
}

Expected Behavior

Behaviour should be documented and better design decision has to be made:

Either:

  • ignore zero as invalid argument: return errWriteTimeout.
  • ignore zero deadline: The maximum possible deadline should be used, not arbitrary value of 1000 hours.
  • respect zero as valid deadline: Fail request immediately.

Steps To Reproduce

Call WriteControl with a zero-deadline but fail to respond for ~41 days.

This is rather theoretical, but a non-conformant websocket or bad firewall might lead to this.

Anything else?

Originally reported in #841 (comment).
Confirmed as "unexpected" by maintainer in #841 (comment).

In practice probably doesn't affect anybody; it's just weird.

@jaitaiwan
Copy link

Champion @JannikGM thank you. I'm taking a look at this now.

@jaitaiwan
Copy link

Nothing in the RFC seems to indicate any sane value for this. I don't know of any websocket extensions that need to be accounted for in any changes here.

It appears to me that this code is used in both client and server connections, so this probably is a more important function when it comes to clients being unable to write to the server.

Based on that information I think that the following actions should be taken:

  1. The default is documented as a warning
  2. Any examples should be updated to include setting this deadline to a sane standard
  3. This issue should be left open and marked for a v2 as changing this could be a nasty surprise for implementors that don't know about this

AlexVulaj pushed a commit that referenced this issue Mar 6, 2024
A zero value for the Conn.WriteControl deadline specifies no timeout,
but the feature was implemented as a very long timeout (1000 hours).
This PR updates the code to use no timeout when the deadline is zero.

See the discussion in #895 for more details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants