keepalive_ping wait_for as task?

[Serpens66]

Currently, if the ping_intervall is 25 seconds and the "asyncio.wait_for" in keepalive_ping takes for whatever reason 10 seconds, it results in a timeframe of 35 seconds with no new ping sent.
Of course this might be exceptional and one should not use a connection with such a delay... but is there any reason to not put this wait_for into a task, so it does not block next lines (which would result in the intended 25 seconds between pings).

I'm no asyncio expert, but from testing this seems to work:

                        asyncio.create_task(asyncio.wait_for(
                            pong_waiter,
                            self.ping_timeout,
                            **loop_if_py_lt_38(self.loop),
                        ))

edit:
not sure though how to catch the TimeoutError now :D
... just thinking:
"wait_for will cancel the pong_waiter future, so we can catch it with add_done_callback and check if it was cancelled"
but then I saw that you are shielding the future to prevent cancellation... is there a reason for this? Why should the pong_waiter not be cancelled when wait_for raises the TimeoutError?
...
or how else can we make sure the ping is sent every x seconds, regardless on how long we wait for the pong?

[aaugustin]

We could record how long it took to send the ping and reduce the wait accordingly. That would improve the behavior as long as sending the ping takes less than ping_interval.

[Serpens66]

We could record how long it took to send the ping and reduce the wait accordingly. That would improve the behavior as long as sending the ping takes less than ping_interval.

If there is no better solution, yes... But I can not imagine asyncio has no better way to handle timeouts without being forced to await it. Is there really no better solution?

[aaugustin]

Your suggestion is also an option, however I don't like creating too many tasks at the library level. There's a long string of concurrency issues in this bug tracker... The fewer tasks, the fewer concurrency issues.

[Serpens66]

Your suggestion is also an option, however I don't like creating too many tasks at the library level. There's a long string of concurrency issues in this bug tracker... The fewer tasks, the fewer concurrency issues.

ok, can you tell me how to catch the timeouterror when creating a task for the wait_for ?
I use a custom keepalive_ping function anyways, so from my view there is no urgent need to change websockets itself regarding this.

[aaugustin]

See the second example here: https://docs.python.org/3/library/asyncio-task.html#asyncio.wait_for

[Serpens66]

See the second example here: https://docs.python.org/3/library/asyncio-task.html#asyncio.wait_for

there is only one example for "wait_for", so no clue what you mean by "second example". If you mean "asyncio.wait(..)" I don't think this helps.

But I found the easy solution, simply put the wait_for in another coroutine and call it by creating a task, like this:

# use create_task to call this fn, so it runs in background
async def wait_for_callastask(self,aw,timeout=None,**kwargs):
    try:
        await asyncio.wait_for(
            aw,
            timeout,
            **loop_if_py_lt_38(self.loop),
        )
    except asyncio.TimeoutError:
        if kwargs.get("name")=="pong_waiter": # so this function can be used for all kinds of awaitables, not only pong_waiter
            self.connection.fail_connection(1011, "keepalive ping timeout")
            
async def keepalive_ping(self) -> None:
    background_tasks = set() # important, see: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task
    # ...
    try:
        while True:
            # ...
            if self.ping_timeout is not None:
                task = asyncio.create_task(self.wait_for_callastask(pong_waiter,self.ping_timeout,name="pong_waiter"))
                background_tasks.add(task)
                task.add_done_callback(background_tasks.discard)
        
        except asyncio.CancelledError:
            raise
        except ConnectionClosed:
            pass
        except Exception:
            self.logger.error("keepalive ping failed", exc_info=True)

I implemented this solution into my custom keepalive, so it is not needed that you also add this.
So feel free to close this error or implement it, just as you want.

---

But a question for understanding:
Is there any reason your "def ping" is making the pong_waiter "shielded" ? In your "def read_data_frame" function you do already check "if not ping.done():", so I see no reason why the pong_waiter should not be cancelled on timeout automatically. Or is there another reason for using shielded?

I ask this, because to keep track of latency ( #1195 ) it seems to be mandatory to add a done_callback to pong_waiter. And this callback is only called when the future is cancelled on timeout. And the "latency" then would be "timeout".

But again, with custom keepalive I already added latency measurement on my own. So also no urgent need for this.
Still I think it would be a good improvement in general.

[aaugustin]

Is there any reason your "def ping" is making the pong_waiter "shielded" ?

Yes.

If a user of websockets awaits a coroutine that awaits pong_waiter, and then cancels that coroutine, pong_waiter shouldn't be cancelled, or else websockets will end up in an internally inconsistent state.

[aaugustin]

The idea of forking one task per ping is rejected. I filed #1240 for scheduling pings more accurately. I think that covers all the points discussed here.