Wow, that was fast!

I do think that it would be a good idea to automatically respond with a pong for every ping received. Just about every web socket implementation I've seen does this by default, and I think its part of the web socket spec -- users probably shouldn't have to worry about responding to pings in their application code.

@andrewhamon Sending PING messages is usually done on the server side. See django-websocket-redis, MDN and related SO discussion.

@Sija yes, but the same WebSocket class is used for both clients and servers if I'm not mistaken. Furthermore, RFC 6455 clearly states that a ping must trigger a pong in response:

The Ping frame contains an opcode of 0x9.

A Ping frame MAY include "Application data".

Upon receipt of a Ping frame, an endpoint MUST send a Pong frame in
response, unless it already received a Close frame. It SHOULD
respond with Pong frame as soon as is practical. Pong frames are
discussed in Section 5.5.3.

An endpoint MAY send a Ping frame any time after the connection is
established and before the connection is closed.

NOTE: A Ping frame may serve either as a keepalive or as a means to
verify that the remote endpoint is still responsive.

The MDN resource you linked also states:

At any point after the handshake, either the client or the server can choose to send a ping to the other party.

This is also how its done in the Gorilla WebSocket library for Go:

The default ping handler sends a pong to the peer. The application's reading goroutine can block for a short time while the handler writes the pong data to the connection.

The implementation could be as simple as:

def initialize(@ws : Protocol)
  @buffer = Slice(UInt8).new(4096)
  @current_message = IO::Memory.new
  on_ping do |msg|
    pong(msg)
  end
end

which would allow the user to easily override the ping handler themselves if they wanted.

@andrewhamon Sure, it totally makes sense, I'd just want an opinion on that from @asterite or someone from @crystal-lang team :)

Yes, this is a required protocol detail that can be automated. Please send a PONG immediately when receiving a PING, unless a CLOSE frame was already received. As per the spec.

@ysbaddaden ATM HTTP::WebSocket::Protocol doesn't track closed state. If you'd like to introduce it, shall it be synchronized with underlying @io?

I agree with @ysbaddaden, I'm not sure we need on_ping and on_pong, we must reply a PING with PONG and that's it

@asterite on_pong is needed to make ping useful. For example:

timeouter = nil
pinger = PeriodicTimer.new(5) do
  ws.ping
  timeouter = Timer.new(4) do
    ws.close # Didn't send back a pong in time
  end
end

ws.on_pong do
    timeouter.cancel
end

Is some pseudocode for disconnecting if a peer is unresponsive for 5 seconds.

@andrewhamon Ah, right

An alternative would be to make ping take a block to execute when a pong comes back.

ws.ping do
  # Execute block when the next pong comes back
end

Which I actually think I like a bit better, since a pong shouldn't be showing up anyways unless there is a ping that triggered it.

@asterite In Ruby world em-websocket for instance exposes onping and onpong callbacks.

Maybe we can expose them, but according to the protocol an incoming PING must always send a PONG, so the ping handler should be able to do something in addition to the default behaviour (that is, not overwrite it). Sounds good?

I'd go with overwrite, and provide a very straightforward way to pong the msg. That way it's easier for a user to perform tasks before and after the pong is sent. Of course, if no on_ping handler is provided, the ws should pong. WDYT @asterite?

Keeping it Simple

I think that the faye-websocket-ruby interface does a good job providing a minimal interface that supports most use cases. Theres no pong, on_ping or on_pong. ping simply accepts a block that gets called when the matching pong comes back, and incoming pings trigger a pong response.

This is a very safe path and would make it really hard for applications to do anything in violation of the protocol, but is still flexible enough for most things.

Allowing more control

OTOH, this is the standard library and the WebSocket class should perhaps be thought of more as a building block, in which case we should allow more.

For example:

A Pong frame MAY be sent unsolicited. This serves as a
unidirectional heartbeat. A response to an unsolicited Pong frame is
not expected.

Which would necessitate an exposed pong method for applications.

We also don't expose binary messages.

Even if we do expose everything, though, the RFC is very clear:

Upon receipt of a Ping frame, an endpoint MUST send a Pong frame

Which makes me unsure whether applications should be able to override the default ping handler and potentially break compliance or provide a ping handler in addition to the default.

I monkey-patched these changes into an app and can confirm that they are working as expected for my use case (heroku router keepalive). Thanks @Sija!

Bump. Any additional thoughts @asterite @Sija?

I'd go with current design + simple overridable #on_ping handler to leave decision when to #pong to the user (or other lib).

I'm not sure though how to proceed with tracking closed state since neither class does it (HTTP::WebSocket::Protocol nor HTTP::WebSocket::Protocol). Shall I introduce it (and synchronize it with underlying @ws.io)?

So I'm reading the RFC and I assume you are talking about this:

an endpoint MUST send a Pong frame in
response, unless it already received a Close frame.

Given this

If an endpoint receives a Close frame and did not previously send a
Close frame, the endpoint MUST send a Close frame in response. (When
sending a Close frame in response, the endpoint typically echos the
status code it received.) It SHOULD do so as soon as practical. An
endpoint MAY delay sending a Close frame until its current message is
sent (for instance, if the majority of a fragmented message is
already sent, an endpoint MAY send the remaining fragments before
sending a Close frame). However, there is no guarantee that the
endpoint that has already sent a Close frame will continue to process
data.

I think its O.K. to not worry about it, as long as the close frame is sent in a timely manner.

tl;dr the RFC doesn't compel implementations to not send a pong after receiving a close, so I don't think the close state needs to be tracked.

In theory this could result in a surprising IO error if a peer:

• sent a ping, and
• sent a close, and
• closed the underlying TCP socket before the ping response

Which I think would be out of spec for the peer.

Furthermore, the run loop executes the callbacks in the same Fiber it seems, so unless the callbacks themselves spawn a Fiber, the only way a pong will go out after a close would be if the peer sent a ping after a close.

@Sija This PR is almost perfect. The only thing missing is always sending a PONG in reply to a PING. This must be hardcoded and always done, because it's a protocol requirement. A user can additionally specify a PING handler to do some extra stuff, in addition to the default PONG reply (though I don't know what's a use case for that, but I'm OK with having that option)

And if the protocol requires sending a CLOSE frame in reply to CLOSE, then I'd do that too (only for the first CLOSE frame, so we'd need to track that with a bool)

I'm not sure that I like silently returning false if closed. Especially since it's not being concistently applied for all message types.

Also, as I said earlier, I don't think it's necessary to track closed at all. Either the underlying IO is still open, and messages are permitted by the RFC (even after receiving a close frame), or the IO is closed and an IO error will be raised.

If you insist on tracking closed, sending a message of any type should raise an exception. Otherwise applications will have a mix of checking for an error return (false) AND exception handeling (since they should be prepared for an IO error anyways)

@andrewhamon uhm, I had that funny feeling about just returning false, I'll push fix to make it raise. Before that, shall we track close state at all or just for first reply to CLOSE frame, WDYT @asterite?

Can you add some docs to the new methods while you're at it? 😊

@andrewhamon I've pushed the updates.

@Sija I just made a final tiny comment about double closing. After that, I'll merge this so it's available in the next version

@asterite done!

Great, thank you! ❤️