Handle ping/pong events in ASGIWebSocketAsyncNetworkStream

[dmontagu]

We've been occasionally hitting errors in CI apparently due to our ASGI app receiving Ping events in our tests, and hitting the error just below the lines added in this PR.

I don't know if there's a reason not to make this change but figured I would propose it. If there's a better solution than what is proposed here, any insight would be appreciated.

[frankie567]

Hey @dmontagu 👋

Good catch indeed. I think the reason why I didn't implement Ping/Pong handling in the transport is because it's not part of the ASGI specification.

I even have a unit test that was relying on Ping message to check the UnhandledWebSocketEvent exception 😅

httpx-ws/tests/test_transport.py

Lines 81 to 91 in cd0a3dc

	async def test_write_unhandled_event(self, scope: Scope):
	async def app(scope, receive, send):
	await send({"type": "websocket.accept"})
	await receive()
	connection = wsproto.connection.Connection(wsproto.connection.CLIENT)
	async with ASGIWebSocketAsyncNetworkStream(app, scope) as (stream, _):
	with pytest.raises(UnhandledWebSocketEvent):
	ping_event = wsproto.events.Ping(b"PING")
	await stream.write(connection.send(ping_event))

---

The easiest solution right now is to set the keepalive_ping_interval_seconds parameter to None on the client to disable the keepalive ping, which should be okay in a CI context.

That being said, as it's enabled by default, it's true that it provokes surprising errors with the ASGI transport. I think it would probably better to automatically disable it if we detect that we work with the ASGI transport rather than trying to handle the ping. What do you think?

Out of curiosity, in which project are you using it? 🙂

[Kludex]

That being said, as it's enabled by default, it's true that it provokes surprising errors with the ASGI transport. I think it would probably better to automatically disable it if we detect that we work with the ASGI transport rather than trying to handle the ping. What do you think?

Yes.

Out of curiosity, in which project are you using it? 🙂

😉

[frankie567]

I made a PR with my proposed solution: #63

If that looks good for you, then I'll merge it, and we can close the current one.

[frankie567]

So, I close this in favor of #63. Will make a release in a moment.

@all-contributors add @dmontagu for bug

[allcontributors]

@frankie567

I've put up a pull request to add @dmontagu! 🎉