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

Expose API to send pings #1

Closed
nhooyr opened this issue Aug 13, 2018 · 15 comments
Closed

Expose API to send pings #1

nhooyr opened this issue Aug 13, 2018 · 15 comments

Comments

@nhooyr
Copy link
Owner

nhooyr commented Aug 13, 2018

https://stackoverflow.com/questions/23238319/websockets-ping-pong-why-not-tcp-keepalive

@nhooyr nhooyr added the p3 label Sep 6, 2018
@nhooyr nhooyr changed the title why support ping/pong? support ping/pong Sep 6, 2018
@nhooyr nhooyr added p1 and removed p3 labels Sep 6, 2018
@nhooyr nhooyr changed the title support ping/pong Support ping/pong Sep 7, 2018
@nhooyr
Copy link
Owner Author

nhooyr commented Sep 8, 2018

To keep the API simpler and prevent it from becoming a wrapper around the net.Conn, its probably best to let applications implement ping/pongs on their own and have canonical examples of how to do this.

@nhooyr nhooyr modified the milestone: v1.0.0 Mar 14, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Mar 16, 2019

No idea what I meant by preventing it from becoming a wrapper around net.Conn.

@nhooyr nhooyr added this to the v1.0.0 milestone Mar 16, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Mar 29, 2019

Given browsers don't implement ping/pong, I'm thinking its unnecessary. If someone else has a good argument otherwise, please let me know. I'd be happy to implement default ping/pong.

@nhooyr nhooyr closed this as completed Mar 29, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Mar 29, 2019

See https://bugs.chromium.org/p/chromium/issues/detail?id=706002 and https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/2RAm-ZYAIYY

The consensus has been its not worth the expense and TCP keep alives are enough, which I agree with.

@nhooyr
Copy link
Owner Author

nhooyr commented Mar 30, 2019

It also wouldn't really make sense over HTTP/2

@ctd1500
Copy link

ctd1500 commented Mar 30, 2019

Given browsers don't implement ping/pong

This isn't quite accurate, browsers do respond to ping with pong, the links you gave are about implementing automatic ping from the browser-side.
Websocket ping/pong implementations are typically designed around the server pinging clients/browsers.

TCP keepalive is not end-to-end and doesn't inform you of the client still being alive, simply that a possible proxy still exists.

@nhooyr
Copy link
Owner Author

nhooyr commented Mar 30, 2019

This isn't quite accurate, browsers do respond to ping with pong, the links you gave are about implementing automatic ping from the browser-side.
Websocket ping/pong implementations are typically designed around the server pinging clients/browsers.

That is true, but I'd argue most WebSocket servers do not send ping/pongs and just use TCP keep alive and things work fine. HTTP/1.1 persistent connections also just use TCP keep alive without major issues. I could be wrong but I believe net/http's HTTP/2 server also does not use HTTP/2 keep alives because TCP keep alives are cheaper and enough.

Its not end to end but most in between proxies also use TCP keep alives because they do not want to keep unnecessary connections up as well.

The other issue I have with this feature is I don't think I should give it an option because most of the time, TCP keep alives will be enough and it won't make sense over HTTP/2 as HTTP/2 has protocol level pings and wouldn't work at all with WASM. I also do not want to make it automatic as it can be expensive.

We can always revisit this later.

@nhooyr
Copy link
Owner Author

nhooyr commented Mar 30, 2019

Although, I'm not 100% sure. I'll reopen this for now for further discussion.

@nhooyr nhooyr reopened this Mar 30, 2019
@nhooyr nhooyr removed the important label Mar 30, 2019
@nhooyr nhooyr changed the title Support ping/pong Should ping/pong be supported? Mar 30, 2019
@nhooyr nhooyr changed the title Should ping/pong be supported? Ping Support Apr 20, 2019
@nhooyr nhooyr changed the title Ping Support Ping support Apr 20, 2019
@nhooyr nhooyr removed this from the v1.0.0 milestone Apr 25, 2019
@nhooyr nhooyr changed the title Ping support Expose API to send pings Apr 27, 2019
@renevall
Copy link

renevall commented May 3, 2019

For my use case, we are running a few IoT devices connected through websocket. We can't guarantee the quality of the connection at all times, so we deal with a fair number of reconnects. Using the ping/pong on the server allow us to free the resources allocated to the connections and be aggressive with ping/pongs. The whole point is we want our server to know in a timely manner if a device got disconnected. Not sure if KeepAlive helps with that purpose.

@nhooyr
Copy link
Owner Author

nhooyr commented May 3, 2019

TCP keep alive is exactly for that purpose, it should be good enough for what you're doing.

@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

No one has commented on this in a while so going to close assuming the lack of a ping/pong API is fine.

@nhooyr nhooyr closed this as completed May 30, 2019
@ctd1500
Copy link

ctd1500 commented May 30, 2019

TCP Keepalive only operates on connections specifically marked "idle" and has no user application level, it only has a kernel level.
It may report to the kernel that a TCP connection still exists, but it cannot in any way tell you that the application is still responding/connected and not deadlocked. Keepalive also only reports after 2 hours, which cannot be changed on many operating systems.
There's a good reason why TCP software implements ping/pong even though Keepalive has existed for decades.

Not to mention analytics for latency between server and client collected from ping/pong.

@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

It may report to the kernel that a TCP connection still exists, but it cannot in any way tell you that the application is still responding/connected and not deadlocked.

Not necessarily true. E.g. with this library, your application could be deadlocked but you'd still respond to pings because this library spawns a readLoop goroutine.

One also ought to have maximum time spent waiting for the next message anyway. So if the application is not responding/connected, the connection will be dced. In this library, this would correspond with the timeout on the context passed to c.Reader. E.g. a reasonable timeout might be 15 minutes which would bound the time between message reads to max 15 minutes. So in practice, I'm not sure if this would matter.

Keepalive also only reports after 2 hours, which cannot be changed on many operating systems.

With Go at least, by default net/http sets a keep alive of 3 minutes so it shouldn't report after 2 hours.

Not to mention analytics for latency between server and client collected from ping/pong.

Solid point, I didn't think about this. Will reopen.

@nhooyr nhooyr reopened this May 30, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

Could add an API like:

type Conn struct {
    // Receive on Pongs to receive WebSocket pongs.
    // The library will never block on sending to Pongs.
    // As such, it has a buffer of 1. If Pongs are not read
    // further pongs will just be dropped.
    // The string is the pong payload.
    Pongs <-chan string
    ...
}

// Ping will send a WebSocket ping to the remote end with the given
// payload.
func (c *Conn) Ping(ctx context.Context, payload string) error {
}

@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

Could also take the HTTP/2 approach: c3mb0/net@780a5dd

nhooyr added a commit that referenced this issue May 30, 2019
- Closes #1 (Ping API)
- Closes #75 (Read/Write convienence methods)
- Closes #83 (SetReadLimit)
nhooyr added a commit that referenced this issue May 30, 2019
- Closes #1 (Ping API)
- Closes #62 (Read/Write convienence methods)
- Closes #83 (SetReadLimit)
nhooyr added a commit that referenced this issue May 30, 2019
- Closes #1 (Ping API)
- Closes #62 (Read/Write convienence methods)
- Closes #83 (SetReadLimit)
@nhooyr nhooyr closed this as completed in 4de9061 May 31, 2019
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

3 participants