Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OkHttpWebsocketSession does not complete the closeReason future when closing the websocket in absence of connectivity #8678

Open
TestaDiRapa opened this issue Feb 19, 2025 · 1 comment
Labels
bug Bug in existing code

Comments

@TestaDiRapa
Copy link

Hello! 👋
I encountered this issue while doing some tests for the reliability of my ktor-based client application.

Reproduction steps

  • Initialize a ktor application with websocket and where the built-in ping-pong mechanism is disabled.
  • Launch 2 coroutines: Coroutine 1 communicates through websocket with a server external to the machine, Coroutine 2 awaits the completion of the closeReason Future through DefaultWebSocketSession.closeReason.
  • Disconnect the machine from the internet.
  • Close the websocket.

Actual Behaviour
Coroutine 1 terminates while Coroutine 2 hangs forever.

Expected Behaviour
Coroutine 2 should also terminate, as closeReason should complete.

Analysis
Looking at the code, my understanding of the reason of this behaviour is that, in the OkHttpWebsocketSession class, the closeReason future is completed only in one of these 3 method: onClosing, onClosed, or onFailure.

  • onFailure is not called by the RealWebSocket implementation when the connection drops because we disabled the built-in ping/pong mechanism in favour of a custom one.
  • onClosing and onClose are not called when the websocket is closed unless a control frame (OPCODE_CONTROL_CLOSE) is sent by the server, event that cannot happen under these conditions.

Additional Information
On the other hand, the CIO engine manages to complete the closeReason future under the same assumptions.

Question
Is this a design choice or just a bug due to a very niche condition?

@TestaDiRapa TestaDiRapa added the bug Bug in existing code label Feb 19, 2025
@jcovington16
Copy link

I would like to take this on if possible @positron @raggi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

No branches or pull requests

2 participants