Skip to content

Ping Behavior Clarification #226

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

Closed
JoshStrobl opened this issue Apr 22, 2020 · 6 comments
Closed

Ping Behavior Clarification #226

JoshStrobl opened this issue Apr 22, 2020 · 6 comments

Comments

@JoshStrobl
Copy link

JoshStrobl commented Apr 22, 2020

Hey there,

I'm currently having some issues understanding what the expected implementation is for handling pings and pongs in a repeated fashion, primarily being used as a "heartbeat" indicator so I can free specific connections so I'm not sending data to socket connections which aren't active. I read over #186 and took a look at both of the examples in the repo and am trying to determine a few things:

I'm using wsjson.Read to read in requested data, as opposed to using the Reader / Read functions. This data is a JSON marshalled form of the struct listed below:

type WebSocketEvent struct {
	IDs  SocketKeys `json:"ids"`
	Type string     `json:"type"`
}

This is being read in as follows:

var request WebSocketEvent
if readErr := wsjson.Read(r.Context(), socketConn, &request); readErr != nil { // Read contents
	return
}

The type has a value, whether that's "init", "register", or "unregister". During "init", we'll perform some mapping and then call a go routine called Ping, providing the pointer the websocket.Conn. The desired implementation is basically to ping every 30 seconds or so and then after a few failures it'll unlisted it.

Is the pong response supposed to be automatically handled by the call to the connection's Ping function? Is this something that would be in the response handler (e.g. func SocketHandler(w http.ResponseWriter, r *http.Request)) instead? Or am I just missing something or going about it the wrong way.

Here's something I was testing, not actually functional but should give some idea on the context:

func Ping(c *websocket.Conn) {
	for {
		ctx, _ := context.WithTimeout(context.Background(), time.Second*15)
		trunk.LogInfo("Pinging client")

		pingErr := c.Ping(ctx)

		if pingErr == nil {
			trunk.LogSuccess("Pinged client")
			time.Sleep(time.Second * 30)
		} else {
			trunk.LogErr("Failed to ping client")
			RemoveConn(socketConn, "all", "all")
			return
		}
	}
}

Thanks in advance for your help!

@nhooyr
Copy link
Contributor

nhooyr commented Apr 22, 2020

Hi,

Is the pong response supposed to be automatically handled by the call to the connection's Ping function

Ping will block until the goroutine in wjson.Read reads the pong from the peer or the deadline is hit.

I've opened #227 to add an example to make this more clear.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 22, 2020

So your code should work as long as Ping runs in a different goroutine than the one in wsjson.Read.

@JoshStrobl
Copy link
Author

JoshStrobl commented Apr 22, 2020

Holy smokes that was a quick response, thanks for the additional information. Here are the two functions, do you see anything obvious that would indicate why at least in Firefox it isn't indicating ping / pong responses in its Messages tab (it seems to receive a ping once, send a pong, then nothing after that).

Screenshot from 2020-04-23 01-06-11

// Ping the client will attempt to regularly ping the client
func Ping(c *websocket.Conn) {
	for {
		ctx, _ := context.WithTimeout(context.Background(), time.Second*15)
		trunk.LogInfo("Pinging client")

		pingErr := c.Ping(ctx)

		if pingErr == nil {
			trunk.LogSuccess("Pinged client")
			time.Sleep(time.Second * 30)
		} else {
			trunk.LogErr("Failed to ping client")
			RemoveConn(socketConn, "all", "all")
		}
	}
}

// SocketHandler will handle any request to registration and unregistration of a WebSocket Connection
func SocketHandler(w http.ResponseWriter, r *http.Request) {
	socketConn, acceptErr := websocket.Accept(w, r, &websocket.AcceptOptions{
		Subprotocols: []string{
			"sbd-v0.1", // 0.1
		},
		InsecureSkipVerify: core.Conf.Servers.WebSockets.DisableOriginVerification,
	})

	if acceptErr != nil {
		return
	}

	var request WebSocketEvent

	if readErr := wsjson.Read(r.Context(), socketConn, &request); readErr != nil { // Read contents
		return
	}

	if request.Type == "init" {
		initErr := SocketInitHandler(socketConn, request)

		if initErr == nil {
			go Ping(socketConn)
		}
	} else if request.Type == "register" { // Register
		SocketRegisterHandler(socketConn, request)
	} else if request.Type == "unregister" { // Unregister
		SocketUnregisterHandler(socketConn, request) // Call to unregister
	} else { // Neither type
		socketConn.Close(websocket.StatusUnsupportedData, "Type specified is invalid")
	}
}

So is wsjson.Read expected to fail unmarshalling the pong given it isn't actually that struct?

@nhooyr
Copy link
Contributor

nhooyr commented Apr 22, 2020

Holy smokes that was a quick response, thanks for the additional information.

🚀

Here are the two functions, do you see anything obvious that would indicate why at least in Firefox it isn't indicating ping / pong responses in its Messages tab (it seems to receive a ping once, send a pong, then nothing after that).

You need to keep reading from the connection in SocketHandler. You can use CloseRead if you don't expect any data messages as is your case.

Also, not sure if it's just the example code you've provided but there are codepaths where the connection isn't closed. I'd recommend using CloseRead after you read the JSON and then calling Ping in the SocketHandler goroutine instead of a separate one.

So is wsjson.Read expected to fail unmarshalling the pong given it isn't actually that struct?

wsjson.Read just tells the Ping goroutine it's received a pong and then continues reading data messages. It doesn't unmarshal the pong.

@nhooyr nhooyr closed this as completed Apr 22, 2020
@JoshStrobl
Copy link
Author

Hey, not to necro this, but after some tinkering I was able to get pinging and the use of the CloseRead to work as I expected thanks to your advise and pointers. So, thank you. Your time is genuinely much appreciated, both working on this as well as providing support.

@nhooyr
Copy link
Contributor

nhooyr commented Apr 23, 2020

Thank you Joshua, I really appreciate that <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants