Skip to content

Conversation

@gpunto
Copy link
Collaborator

@gpunto gpunto commented Sep 16, 2025

Goal

We were missing onFailure handling during socket connection so, in case of errors there, we were never completing the coroutine

Implementation

Override onFailure for the StreamWebSocketListener used during connection.

Testing

Try launching the sample without network connection and verify that the suspend function throws instead of never completing.

Checklist

  • Issue linked (if any)
  • Tests/docs updated
  • I have signed the Stream CLA (required for external contributors)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2025

PR checklist ✅

All required conditions are satisfied:

  • Title length is OK (or ignored by label).
  • At least one pr: label exists.
  • Sections ### Goal, ### Implementation, and ### Testing are filled.

🎉 Great job! This PR is ready for review.

@gpunto gpunto added the pr:bug Bug fix label Sep 16, 2025
@gpunto gpunto force-pushed the fix/handle-socket-connection-failure branch from 407d817 to f44391b Compare September 16, 2025 10:54
@gpunto gpunto requested a review from Copilot September 16, 2025 10:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug where socket connection failures during handshake were not properly handled, causing coroutines to never complete. The fix overrides the onFailure method in the StreamWebSocketListener to properly propagate connection failures.

  • Added onFailure handling in the handshake WebSocket listener to complete coroutines on connection errors
  • Added comprehensive test coverage for the socket connection failure scenario

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
StreamSocketSession.kt Added onFailure override to handle socket connection failures during handshake
StreamSocketSessionTest.kt Added test case and updated imports to verify proper failure handling behavior

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@gpunto gpunto force-pushed the fix/handle-socket-connection-failure branch from f44391b to 1a0367e Compare September 16, 2025 10:54
@gpunto gpunto marked this pull request as ready for review September 16, 2025 10:56
Copy link
Contributor

@VelikovPetar VelikovPetar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
This reminded me, is closing the socket in this scenario a possible occurrence? I cannot think of a case where onClosed would be called here, but perhaps we should handle it as well - WDYT?

@gpunto
Copy link
Collaborator Author

gpunto commented Sep 19, 2025

Looks good to me! This reminded me, is closing the socket in this scenario a possible occurrence? I cannot think of a case where onClosed would be called here, but perhaps we should handle it as well - WDYT?

Good point! What do you think we should do in that case? Resume with a custom exception?

Edit: done, let me know if it makes sense!

@aleksandar-apostolov aleksandar-apostolov merged commit 7d49714 into develop Sep 19, 2025
5 checks passed
@aleksandar-apostolov aleksandar-apostolov deleted the fix/handle-socket-connection-failure branch September 19, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants