WriteControl() changes WriteDeadline permanently

[ayjayt]

I will, later, dig out more information to support this.

https://github.com/gorilla/websocket/blob/9111bb834a68b893cebbbaed5060bdbc1d9ab7d2/conn.go#L463C4-L463C35

func (*Conn) WriteControl ¶
func (c *Conn) WriteControl(messageType int, data []byte, deadline time.Time) error
WriteControl writes a control message with the given deadline. The allowed message types are CloseMessage, PingMessage and PongMessage.

The issue here that the documentation implies that the deadline is set for this one message, whereas it actually calls SetWriteDeadline() and changes the deadline for all subsequent writes as well. It also uses a non-documented default value if supplied time zero &time.Time{} so you should expect this function to change your configuration.

[ghost]

The network connection's SetWriteDeadline method is called from WriteControl and the internal method used by all other writes. These SetWriteDeadline calls establish the appropriate deadline before the network connection's Write method is called.

A bug this severe seemed unlikely in this popular package. I found the information above by searching for SetWriteDeadline in the code.

[ghost]

The deadline default value is interesting. It is idiomatic in Go to use a zero time value to specify no deadline. The WriteControl method uses the zero time value to specify a deadline 1000 hours in the future.

There's not a practical difference between no deadline and a deadline 1000 hours in the future, so this is not something that will have an impact on applications.

It is strange.

[ayjayt]

A bug this severe seemed unlikely in this popular package.

The bug isn't that they don't... the bug is that all write calls call SetWriteDeadline in a way that overwrite the user's calls to SetWriteDeadline, but the function descriptions in the docs imply that the write deadline would be set for that single write.

myConn.SetWriteDeadline(myTime)
myConn.WriteControl(websocket.PingMessage, []byte("Hello"), 200 * time.Millisecond) 
// WriteControl is the same as calling SetWriteDeadline, but it's counterintuitive based on docs.

[ayjayt]

The solution is to change the documentation but also accept that accepting a write deadline in the write helper function was unnecessary, let the developer use SetWriteDeadline. Explicit is good.

[ghost]

The package works as currently documented.

The websocket connection SetWriteDeadline method stores that deadline in a field. The internal write function calls the network connection SetWriteDeadline with the value from the field.

The WriteControl method calls the network connection SetWriteDeadline method before writing to the network connection.

The documented deadline is used in both code paths.

myConn.SetWriteDeadline(myTime) // stores myTime in field
myConn.WriteControl(websocket.PingMessage, []byte("Hello"), 200 * time.Millisecond) // does not change deadline in field
myConn.WriteMessage(...) // uses deadline from field

[ayjayt]

Also, documenting a poor design decision does NOT qualify it as a feature.

[ghost]

Why is this tagged as a bug and still open?
The originally documented issue assumes SetWriteDeadline is called on the gorilla websocket object, but it's called on the underlying socket instead, so there is no bug.

This was already explained in:

• WriteControl() changes WriteDeadline permanently #841 (comment)
• WriteControl() changes WriteDeadline permanently #841 (comment)

The other issue about 0 deadline being 1000 hours is weird, but that'd be a separate issue (#895).

[jaitaiwan]

Thanks for the comment @JannikGM, I'll close this one out, but agree based on the feedback in this thread that the behaviour is unexpected.

[ghost]

Unexpected? Deadline management works as documented and would be useless if it worked the way OP assumes that it does.

[jaitaiwan]

I was referring to

0 deadline being 1000 hours

Thanks

[ghost]

Sorry. Yes, that is weird.