search

frankie567 / httpx-ws

WebSocket support for HTTPX
https://frankie567.github.io/httpx-ws/
MIT License
110 stars 17 forks source link

Handle ping/pong events in `ASGIWebSocketAsyncNetworkStream` #59

Closed dmontagu closed 8 months ago

dmontagu commented 8 months ago

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 commented 8 months ago

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 😅

https://github.com/frankie567/httpx-ws/blob/cd0a3dc7e9511bf926122332b3091664ea814584/tests/test_transport.py#L81-L91

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 commented 8 months ago

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 commented 8 months ago

I made a PR with my proposed solution: https://github.com/frankie567/httpx-ws/pull/63

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

frankie567 commented 8 months ago

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

@all-contributors add @dmontagu for bug

allcontributors[bot] commented 8 months ago

@frankie567

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