Skip to content

[refactor] More robust cleanup for websocket connection#11549

Merged
sfc-gh-bnisco merged 1 commit intodevelopfrom
bnisco/fix-flaky-ws-test
Jun 6, 2025
Merged

[refactor] More robust cleanup for websocket connection#11549
sfc-gh-bnisco merged 1 commit intodevelopfrom
bnisco/fix-flaky-ws-test

Conversation

@sfc-gh-bnisco
Copy link
Copy Markdown
Collaborator

@sfc-gh-bnisco sfc-gh-bnisco commented Jun 6, 2025 •
edited
Loading

Describe your changes

  • Adds an explicit cleanup function to the doInitPings function. Wires up the source to call into it.
  • This should fix the vitest errors we were seeing: 
    ⎯⎯⎯⎯⎯ Uncaught Exception ⎯⎯⎯⎯⎯
ReferenceError: window is not defined
 ❯ connect connection/src/DoInitPings.tsx:134:31
 ❯ Timeout.retryImmediately [as _onTimeout] connection/src/DoInitPings.tsx:66:5
 ❯ listOnTimeout node:internal/timers:588:17
 ❯ processTimers node:internal/timers:523:7

This error originated in "connection/src/WebsocketConnection.test.tsx" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
This error was caught after test environment was torn down. Make sure to cancel any running tasks before test finishes:
- cancel timeouts using clearTimeout and clearInterval
- wait for promises to resolve using the await keyword

GitHub Issue Link (if applicable)

Testing Plan

  • ✅ Unit Tests (JS and/or Python) - This updates JS unit tests

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-bnisco sfc-gh-bnisco added security-assessment-completed impact:internal PR changes only affect internal code change:refactor PR contains code refactoring without behavior change labels Jun 6, 2025
@snyk-io
Copy link
Copy Markdown
Contributor

snyk-io bot commented Jun 6, 2025 •
edited
Loading

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 6, 2025 •
edited
Loading

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-11549/streamlit-1.45.1-py3-none-any.whl
🕹️ Preview app pr-11549.streamlit.app (☁️ Deploy here if not accessible)

@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot June 6, 2025 19:37

This comment was marked as outdated.

Sign in to view

@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot June 6, 2025 19:48

This comment was marked as outdated.

Sign in to view

@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot June 6, 2025 19:50

This comment was marked as outdated.

Sign in to view

@sfc-gh-bnisco sfc-gh-bnisco force-pushed the bnisco/fix-flaky-ws-test branch from a4daf15 to 58ee871 Compare June 6, 2025 19:54
@sfc-gh-bnisco sfc-gh-bnisco requested a review from Copilot June 6, 2025 19:54
Copilot AI reviewed Jun 6, 2025
View reviewed changes
Copy link
Copy Markdown
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 refactors the WebSocket connection cleanup logic to make the ping mechanism more robust by introducing an async ping request object with cancellation support. Key changes include:

  • Returning an object with both promise and cancel from doInitPings.
  • Updating the WebsocketConnection class to store and manage an AsyncPingRequest.
  • Refactoring associated tests to destructure and await the promise from the new ping API.

Reviewed Changes

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

File Description
frontend/connection/src/WebsocketConnection.tsx Updated pingServer and disconnect methods to use an AsyncPingRequest.
frontend/connection/src/WebsocketConnection.test.tsx Modified tests to extract the promise from the new doInitPings return.
frontend/connection/src/DoInitPings.tsx Changed the return type to AsyncPingRequest and added a cancel method.
Comments suppressed due to low confidence (2)

frontend/connection/src/WebsocketConnection.tsx:342

  • Consider adding an inline comment explaining the rationale behind storing the full AsyncPingRequest (with both promise and cancel) on the connection. This would aid maintainability and clarify the new ping cancellation logic.
this.pingRequest = doInitPings(

frontend/connection/src/WebsocketConnection.test.tsx:963

  • It would be beneficial to add a test case that verifies the ping request is cancelled when disconnect() is called, ensuring the new cancellation behavior is properly covered.
client.disconnect()

@sfc-gh-bnisco sfc-gh-bnisco marked this pull request as ready for review June 6, 2025 20:50
sfc-gh-nbellante
sfc-gh-nbellante approved these changes Jun 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@sfc-gh-nbellante sfc-gh-nbellante left a comment

Choose a reason for hiding this comment

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

lgtm

@sfc-gh-bnisco sfc-gh-bnisco merged commit e026880 into develop Jun 6, 2025
40 of 41 checks passed
@sfc-gh-bnisco sfc-gh-bnisco deleted the bnisco/fix-flaky-ws-test branch June 6, 2025 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@sfc-gh-nbellante sfc-gh-nbellante sfc-gh-nbellante approved these changes

Assignees

No one assigned

Labels

change:refactor PR contains code refactoring without behavior change impact:internal PR changes only affect internal code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sfc-gh-bnisco @sfc-gh-nbellante