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

Context expiration on Read shuts down WebSocket #242

Closed
misberner opened this issue May 23, 2020 · 1 comment
Closed

Context expiration on Read shuts down WebSocket #242

misberner opened this issue May 23, 2020 · 1 comment
Labels
Milestone

Comments

@misberner
Copy link

If the context I pass to (*Conn).Read expires, this shuts down the entire websocket. conn_notjs:160 is at fault here.

This is confusing and severely constraints the ways in which this library can be used. For example, the following is not possible:

for {
  readCtx, cancel := context.WithTimeout(readCtx, 10 * time.Second)
  mt, msg, err := conn.Read(readCtx)
  cancel()
  if err == context.DeadlineExceeded {
    fmt.Println("No message so far. Don't worry, connection is still alive, let's wait another 10s.")
    continue
  }
}

It is also extremely un-idiomatic since contexts communicate expectations/bounds by the caller only, and the effects of their expiration should be local to the call.

The only way to work around this would be to move all reads in a goroutine and communicate the messages read back via a channel, which is.. meh.

@nhooyr nhooyr added the docs label May 24, 2020
@nhooyr
Copy link
Contributor

nhooyr commented May 24, 2020

100% agree this is unfortunate and definitely not a design decision but the API exposed to us by *http.Client is a io.ReadWriteCloser. The only way we can block an in progress Read or Write is by closing the connection. Later on when http2 support is in, this limitation will likely still exist both server and client side. See #4 and the associated Go issues for details.

It is also extremely un-idiomatic since contexts communicate expectations/bounds by the caller only, and the effects of their expiration should be local to the call.

I agree but there isn't a good way yet to bound a context to a IO operation so unfortunately this is the best that can be done.

The only way to work around this would be to move all reads in a goroutine and communicate the messages read back via a channel, which is.. meh.

Yes, it's unfortunate but that is your best shot at the moment, shouldn't be much boilerplate. In my experience such patterns are uncommon compared to just closing the connection if there hasn't been a message sent in X amount of time. If you really need that, I'd go with gorilla/websocket or gobwas/ws, I know gobwas/ws is particularly optimized for this.

I'll document the decisions here better for future users.

oidq added a commit to oidq/disgord that referenced this issue May 31, 2020
* fix handling of error in nhooyr websocket wrapper (mentioned in andersfylling#298)
	* closing context given to g.c.Read(ctx) leads to WebSocket being closed
  	  coder/websocket#242
@nhooyr nhooyr added this to the v1.8.7 milestone Jul 5, 2020
nhooyr added a commit that referenced this issue Jan 9, 2021
@nhooyr nhooyr closed this as completed Jan 9, 2021
philippseith added a commit to philippseith/signalr that referenced this issue Oct 21, 2021
webSocketConnection Read and Write had separate
timeouts which, when expired, mutually closed the
websocket. In case only one party of the connection
was sending, the websocket was closed after the
timeout despite the fact that the connection was alive.
This comes from misunderstanding the impact of the Read/Write context parameter. See coder/websocket#242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants