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.

Sign up for GitHub

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

Jump to bottom

Add ping/pong to websocket protocol #995

Merged
merged 2 commits into from
Feb 25, 2016
Merged

Add ping/pong to websocket protocol #995

merged 2 commits into from
Feb 25, 2016

Conversation

paulbellamy
Copy link
Contributor

Fixes #828

Also made the websocket codec WaitForReadError return the error.

@tomwilkie
Copy link
Contributor

Also needed for the pipes websocket, no?

And do we ever call codec.Close()?

@paulbellamy
Copy link
Contributor Author

re: pipes. probably, actually.

codec.Close() is called from the rpc client here: https://golang.org/src/net/rpc/client.go?s=7387:7422#L267

@paulbellamy paulbellamy force-pushed the 828-websocket-ping branch 4 times, most recently from acc9c75 to 83222d7 Compare February 22, 2016 14:57
@paulbellamy
Copy link
Contributor Author

@tomwilkie, added ping to all probe/app websockets (I think), wdyt?

@tomwilkie tomwilkie assigned paulbellamy and unassigned tomwilkie Feb 23, 2016
paulbellamy
paulbellamy reviewed Feb 23, 2016
View reviewed changes
common/xfer/websocket.go
}

// Ping adds a periodic ping to a websocket connection.
func Ping(c *websocket.Conn) Websocket {

This comment was marked as abuse.

Sign in to view

@paulbellamy
Copy link
Contributor Author

Tested this by opening a websocket (via nginx) to /api/control/ws, and seeing it be terminated after 60s. With this patch (and xfer.Ping added on the server-side instead of the client-side), it stays open indefinitely.

Edit: rebased and squashed.

@paulbellamy paulbellamy assigned tomwilkie and unassigned paulbellamy Feb 24, 2016
tomwilkie
tomwilkie reviewed Feb 25, 2016
View reviewed changes
app/api_topology.go
if err != nil {
// log.Info("Upgrade:", err)
return
}
conn := xfer.Ping(wsConn)

This comment was marked as abuse.

Sign in to view

@tomwilkie
Copy link
Contributor

Looking better. If I understand this right, the probe will now ping the app and the app ping the probe? Not problem with that I guess.

@tomwilkie
Copy link
Contributor 

./common/xfer/websocket.go:67:1: comment on exported function DialWS should be of the form "DialWS ..."

tomwilkie
tomwilkie reviewed Feb 25, 2016
View reviewed changes
common/xfer/websocket.go
@@ -44,5 +156,25 @@ func ReadJSONfromWS(c *websocket.Conn, v interface{}) error {
// One value is expected in the message.
err = io.ErrUnexpectedEOF
}
if err == nil {
err = p.conn.SetReadDeadline(mtime.Now().Add(pongWait))
}

This comment was marked as abuse.

Sign in to view

tomwilkie
tomwilkie reviewed Feb 25, 2016
View reviewed changes
common/xfer/websocket.go
// Ping adds a periodic ping to a websocket connection.
func Ping(c *websocket.Conn) Websocket {
p := &pingingWebsocket{conn: c}
p.conn.SetPongHandler(p.pong)

This comment was marked as abuse.

Sign in to view

@tomwilkie
Copy link
Contributor

LGTM once tests pass.

@tomwilkie tomwilkie removed their assignment Feb 25, 2016
paulbellamy added a commit that referenced this pull request Feb 25, 2016
@paulbellamy
Merge pull request #995 from weaveworks/828-websocket-ping
1d17a33 
Add ping/pong to websocket protocol
@paulbellamy paulbellamy merged commit 1d17a33 into master Feb 25, 2016
@paulbellamy paulbellamy deleted the 828-websocket-ping branch February 25, 2016 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulbellamy @tomwilkie