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

Fix: DO-5316 originating WebSocket channel being notified about their own variable updates #395

Merged

Conversation

krzysztof-causalens
Copy link
Collaborator

@krzysztof-causalens krzysztof-causalens commented Feb 12, 2025

Motivation and Context

When using the BackendStore persistence for Variables, the user who caused the update would also receive a WS message about the change. This would cause potential race conditions and state desyncs where if the variable changes multiple times while an update is being sent to the backend, the client can then get set 'back in time' after they receive a delayed notification about their own update.

This is especially noticeable on slower connections and when storing objects in the variables (as Recoil might skip updates for same primitive values).

Implementation Description

Added ws_channel parameter to the request for POST /api/core/store. We then set the context var WS_CHANNEL before invoking store.write().

Inside the backend store notification logic, we pass the current WS_CHANNEL into the websocket manager's broadcast and send_message_to_user methods as an ignore_channel kwarg.

Implemented the aforementioned ignore_channel kwarg to skip sending updates to a provided channel.

Any new dependencies Introduced

How Has This Been Tested?

Verified in a local app that WS updates are sent to a different tab of the same user but not to themselves.

Updated client- and backend-side tests to cover the new behaviour.

PR Checklist:

  • I have implemented all requirements? (see JIRA, project documentation).
  • I am not affecting someone else's work, If I am, they are included as a reviewer.
  • I have added relevant tests (unit, integration or regression).
  • I have added comments to all the bits that are hard to follow.
  • I have added/updated Documentation.
  • I have updated the appropriate changelog with a line for my changes.

Screenshots (if appropriate):

Copy link
Contributor

@sam-causalens sam-causalens left a comment

Choose a reason for hiding this comment

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

LGTM, nice one 👍

@krzysztof-causalens krzysztof-causalens merged commit 7791356 into master Feb 12, 2025
7 checks passed
@krzysztof-causalens krzysztof-causalens deleted the DO-5316-backend-store-should-not-send-to-self branch February 12, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants