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

SetDeadline should only close the connection if there is an in progress Read/Write #228

Closed
emersion opened this issue Apr 23, 2020 · 2 comments
Milestone

Comments

@emersion
Copy link
Contributor

I'm using websocket.NetConn. net.Conn docs say:

A deadline is an absolute time after which I/O operations fail with a timeout (see type Error) instead of blocking.

However this closes the connection 10s after the write, even if the write has already completed:

conn.SetWriteDeadline(time.Now().Add(10 * time.Second)
conn.Write([]byte("hello world"))
@nhooyr
Copy link
Contributor

nhooyr commented Apr 23, 2020

This is documented on websocket.NetConn.

Unfortunately nothing can be done about it as closing the connection is the only way to unblock any reading/writing goroutines as this library treats the underlying connection as io.ReadWriteCloser even if it's a net.Conn.

The reasoning behind this is that net/http's Client only gives us back a io.ReadWriteCloser for HTTP upgrades.

Another reason is that when http2 upgrades are supported, we'd also only have access to a io.ReadWriteCloser on both the client and server.

So while on the server we could theoretically access the SetDeadline methods on the net.Conn for HTTP/1, it's best not to depend on them at all.

@nhooyr nhooyr closed this as completed Apr 23, 2020
@nhooyr
Copy link
Contributor

nhooyr commented Apr 23, 2020

Actually I see your point, we could only close the connection if there is an in progress read/write but otherwise we don't have to.

@nhooyr nhooyr reopened this Apr 23, 2020
@nhooyr nhooyr changed the title SetWriteDeadline shouldn't close the connection on successful write SetWriteDeadline should only close the connection if there is an in progress Read/Write Apr 24, 2020
@nhooyr nhooyr changed the title SetWriteDeadline should only close the connection if there is an in progress Read/Write SetDeadline should only close the connection if there is an in progress Read/Write Apr 24, 2020
@nhooyr nhooyr added this to the v1.8.7 milestone May 18, 2020
nhooyr added a commit that referenced this issue May 18, 2020
NetConn has to close the connection to interrupt in progress reads
and writes. However, it can error out on reads and writes that occur
after the deadline instead of closing the connection.

Closes #228
nhooyr added a commit that referenced this issue May 18, 2020
NetConn has to close the connection to interrupt in progress reads
and writes. However, it can error out on reads and writes that occur
after the deadline instead of closing the connection.

Closes #228
nhooyr added a commit that referenced this issue May 18, 2020
NetConn has to close the connection to interrupt in progress reads
and writes. However, it can block reads and writes that occur
after the deadline instead of closing the connection.

Closes #228
@nhooyr nhooyr closed this as completed Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants