ping/pong: received pong data frame is discarded

[gowhari]

The received pong data is always None

pong_waiter = await ws.ping(data=custome_ping_data())
print(await pong_waiter)  # always None

It's because handler of OP_PONG always set result of the ping future to None:

https://github.com/aaugustin/websockets/blob/c4a4b6f45af607431c5707d56420bbaf471bbb6e/src/websockets/legacy/protocol.py#L1131

elif frame.opcode == OP_PONG:
    if frame.data in self.pings:
        # Sending a pong for only the most recent ping is legal.
        # Acknowledge all previous pings too in that case.
        ping_id = None
        ping_ids = []
        for ping_id, ping in self.pings.items():
            ping_ids.append(ping_id)
            if not ping.done():
                ping.set_result(None)
            if ping_id == frame.data:
                break

It could be something like this:

ping.set_result(frame.data if ping_id == frame.data else None)

So caller can check ping data in pong result if needed.

Yes if we put await pong_waiter right after ping() it's exactly same data that we send in ping, but this is not always the case.

[aaugustin]

That's a valid improvement request. I didn't expect demand for this — it took 9 years to get this request — but I don't see much harm in implementing it. It will be a backwards incompatible change.

[Serpens66]

+1 I also would like to see this :)

I think this is even mandatory to add latency ( https://github.com/aaugustin/websockets/issues/1195 ) , because eg. one could send the current timestamp as data and when receiving the pong calculate with this how long it took (or put any other data in ping that helps to identify when you sent this ping)

Do you have an ETA on when this (frame.data) may be added to the released version?

[aaugustin]

I tried to implement this and ran into a complication. WebSocketCommonProtocol.ping() accepts either str of bytes. If it receives a str, it encodes it to bytes.

If we want to return the payload of the pong that answered this ping to the user, then all options look bad :-(

(1) Always return bytes.

Then we have an asymmetry if the user does await ws.ping("<str>"). They get back the same value as bytes, UTF-8 encoded: b"<str>"

(2) Magically decode to str when it's UTF-8, else keep bytes.

I'm doing this in the logs. It isn't reliable. Since pings are usually short, too often, a binary string is accidentally valid UTF-8 and treated as a string. That might be acceptable in logs; definitely not in the public API.

(3) Memorize the type that was passed in and return str or bytes accordingly.

Unfortunately, this doesn't work in general. Example:

• in a client, the user does await ws.ping("<str>")
• websockets does await ws.ping(b"bytes") — the keepalive mechanism does this
• the server chooses to answer only the second ping — this is explicitly allowed by the RFC
• we get b"bytes" as the payload of the pong answering the "<str>" ping, and we cannot always return a str

(4) Like (3), but return None when a ping is acknowledged by a later pong, so there's no type mismatch.

This is the original suggestion (ping.set_result(frame.data if ping_id == frame.data else None)) but I'm a unclear on the justification ("So caller can check ping data in pong result if needed."). Could you clarify the use case?

I believe that this API breaks the Principle Of Least Astonishment i.e. "99.99% of the time, I get back the content of my ping, and in 0.01% of cases I get None; WTF?"

[Serpens66]

Not sure if this is what you mean with 2) , but ws4py does it like this: https://github.com/Lawouach/WebSocket-for-Python/blob/a3e6d157b7bb1da1009e66aa750170f1c07aa143/ws4py/messaging.py#L12 (putting the bytes into PongControlMessage which I can do str(pong) to get the string.)

Don't know if also with ws4py one can get "None" in 0.01% of cases, but if so a simple "if pong is not None" in users code solves it, doesn't it?

The usecase is the latency measurement as I wrote in my previous comment. With ws4py I simply put a timestamp into the "ping" and then receive the very same timestamp in the "pong" from the server. To find out how long the the whole process took, I simply can substract this timestamp from the current one, done ( and save this value in self.latency)

[aaugustin]

I just added a built-in solution to measure latency in #1195. For this use case, no further changes are needed.

---

Upon further thought, my initial answer was incorrect. If you need the payload that was sent in the ping when you receive a pong, the obvious solution is:

my_payload = "here's a cool payload!"
pong_waiter = await ws.ping(my_payload)
await pong_waiter
# here you still have `my_payload` available in a local variable

This embraces the coroutine-based design of websockets.

---

Now, if you don't want to do that, you can always fallback to the lower-level option of working with futures and callbacks. Here's one solution, among many others possibilities:

import functools

def do_something_on_pong(payload):
    ...

my_payload = "here's a cool payload!"
pong_waiter = await ws.ping(my_payload)
pong_waiter.add_done_callback(
    functools.partial(do_something_on_pong, my_payload))

If you wanted to implement latency measurement with this pattern in the current release of websockets (where it isn't built in), you could do this:

import time

def record_latency(ws, ping_sent_at):
    pong_rcvd_at = time.perf_counter()
    ws.latency = pong_rcvd_at - ping_sent_at

ping_sent_at = time.perf_counter()
pong_waiter = await ws.ping()
pong_waiter.add_done_callback(
    functools.partial(record_latency, ws, ping_sent_at))

(I didn't test that code but it shouldn't be too far from working code!)