[MM-57737] Improve client side call state consistency #681

streamer45 · 2024-04-05T20:56:11Z

Summary

PR should fix most of the remaining call state data races originally mentioned in #512, specifically when loading the pop out window and in case of a websocket reconnect.

This is done by implementing a websocket request handler that returns the calls state under lock. Using the websocket channel as opposed to making HTTP request should guarantee a valid sequencing of potentially concurrent events (e.g. users joining, leaving, etc).

Ticket Link

https://mattermost.atlassian.net/browse/MM-57737

streamer45 · 2024-04-05T20:57:20Z

webapp/src/index.tsx

@@ -714,59 +715,14 @@ export default class Plugin {
            }
        };

-        const fetchChannelData = async (channelID: string) => {


We can finally get rid of this, it was totally redundant as we'd be fetching the same again in fetchChannels.

streamer45 · 2024-04-05T20:58:07Z

webapp/src/index.tsx

+                    if (skipChannelID === data[i].channel_id) {
+                        logDebug('skipping channel from state loading', skipChannelID);
+                        continue;
+                    }


This is important as it will avoid messing the state for the current call which should only come from websocket from now on.

Can we add a comment in the code to remind us about the what and why? I can imagine forgetting and then spending time trying to reason through it.

codecov-commenter · 2024-04-05T21:02:22Z

Codecov Report

Attention: Patch coverage is 43.18182% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 16.14%. Comparing base (788d6eb) to head (bdcf3f2).
Report is 1 commits behind head on MM-42464.

Files	Patch %	Lines
server/websocket.go	43.18%	23 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           MM-42464     #681      +/-   ##
============================================
+ Coverage     15.91%   16.14%   +0.22%     
============================================
  Files            38       38              
  Lines          6414     6453      +39     
============================================
+ Hits           1021     1042      +21     
- Misses         5271     5286      +15     
- Partials        122      125       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cpoile

This is kind of a weird one isn't it. But if it works 🤷 :)

cpoile · 2024-04-10T14:28:02Z

server/websocket.go

+	if err != nil {
+		return fmt.Errorf("failed to lock call: %w", err)
+	}
+	defer p.unlockCall(channelID)


Would it make much of a difference if we immediately unocked? Looks like there's no reason no to, and it might reduce some of the non-idealness (and prevent the publish ws event from delaying the unlock).

We have ways to get the call state without locking but the point here is that we need to queue the event to be sent through WS before unlocking otherwise we are subject to a race again.

Ah, of course 🤦

cpoile · 2024-04-10T14:32:34Z

server/websocket.go

+		switch msg.Type {
+		case clientMessageTypeJoin, clientMessageTypeLeave, clientMessageTypeReconnect, clientMessageTypeCallState:
+		default:
+			return


As an aside, this is just bad language design. (I mean all c-derivatives here, not just go). Imagine you are a non-programmer (or a programmer who's used to fallthrough, or not used to fallthrough, really), is this quick to read through and know immediately what's happening? sheesh.

yeah, I can revert to crazy if conditions if you prefer, just looked cleaner but not a big deal.

Nah, no big deal, just complaining.

webapp/src/components/expanded_view/component.tsx

cpoile · 2024-04-10T14:44:34Z

webapp/src/components/expanded_view/component.tsx

+        // making a potentially racy HTTP call and should guarantee
+        // a consistent state.
+        logDebug('requesting call state through ws');
+        this.context.sendMessage('custom_com.mattermost.calls_call_state', {channelID: callsClient.channelID});


I wonder if the websocket message isn't a bit confusing. Are you sending the call's state, or requesting it...?

Just being consistent with other events there. We are using the direction (from/to client) to implicitly define whether it's request or response. I know you don't love it :p

I guess HTTP fixes the confusion through the Method. Here we use a bit of context as it doesn't make any sense for the client to ever send the call state. Please let me know if this is blocking :)

Nope, not blocking :)

cpoile · 2024-04-10T14:46:33Z

webapp/src/index.tsx

+                    if (skipChannelID === data[i].channel_id) {
+                        logDebug('skipping channel from state loading', skipChannelID);
+                        continue;
+                    }


Can we add a comment in the code to remind us about the what and why? I can imagine forgetting and then spending time trying to reason through it.

cpoile · 2024-04-10T14:46:42Z

webapp/src/index.tsx

@@ -714,59 +715,14 @@ export default class Plugin {
            }
        };

-        const fetchChannelData = async (channelID: string) => {


cpoile · 2024-04-10T14:47:39Z

webapp/src/index.tsx

+            // We pass currentCallChannelID so that we
+            // can skip loading its state as a result of the HTTP calls in
+            // fetchChannels since it would be racy.


ah, here it is. Maybe can we add a similar comment to the function also?

cpoile · 2024-04-10T14:50:04Z

webapp/src/index.tsx

+        // A dummy React component so we can access webapp's
+        // WebSocket client through the provided hook. Just lovely.


Will this cause us to depend on a new min server version?

This has been there for a long time (before monorepo) so I think we are good. But of course I'd like to fix it properly one day (pass the client object directly to the init function) in which case we'll have to add some backwards compatibility check.

streamer45 · 2024-04-10T15:03:38Z

This is kind of a weird one isn't it.

Why weird? It's a rather simple race condition, our redux state is consistent only if events are received and dispatched in the order they are generated on the server side. With a mixture of HTTP calls and websockets both affecting state we make it racy by design :p

cpoile

Thanks!

streamer45 added the 2: Dev Review Requires review by a core committer label Apr 5, 2024

streamer45 requested a review from cpoile April 5, 2024 20:56

streamer45 self-assigned this Apr 5, 2024

streamer45 commented Apr 5, 2024

View reviewed changes

streamer45 added this to the v0.27.0 / MM 9.9 milestone Apr 9, 2024

cpoile reviewed Apr 10, 2024

View reviewed changes

streamer45 requested a review from cpoile April 10, 2024 16:14

cpoile approved these changes Apr 10, 2024

View reviewed changes

cpoile added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Apr 10, 2024

Base automatically changed from MM-42464 to main April 11, 2024 19:05

streamer45 added 3 commits April 11, 2024 13:09

Improve client side call state consistency

f683984

Fetch config

ff40268

Add comment for skipping loading call state

e039342

streamer45 force-pushed the MM-57737 branch from bb84378 to e039342 Compare April 11, 2024 19:10

streamer45 merged commit 5095fd5 into main Apr 11, 2024
7 checks passed

streamer45 deleted the MM-57737 branch April 11, 2024 21:22

streamer45 mentioned this pull request Apr 11, 2024

Fix fetching call state in popout #695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-57737] Improve client side call state consistency #681

[MM-57737] Improve client side call state consistency #681

streamer45 commented Apr 5, 2024

streamer45 Apr 5, 2024

cpoile Apr 10, 2024

streamer45 Apr 5, 2024

cpoile Apr 10, 2024

codecov-commenter commented Apr 5, 2024 •

edited

Loading

cpoile left a comment

cpoile Apr 10, 2024

streamer45 Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

streamer45 Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

streamer45 Apr 10, 2024

streamer45 Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

cpoile Apr 10, 2024

streamer45 Apr 10, 2024

streamer45 commented Apr 10, 2024

cpoile left a comment

		// A dummy React component so we can access webapp's
		// WebSocket client through the provided hook. Just lovely.

[MM-57737] Improve client side call state consistency #681

[MM-57737] Improve client side call state consistency #681

Conversation

streamer45 commented Apr 5, 2024

Summary

Ticket Link

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2024 • edited Loading

Codecov Report

cpoile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

streamer45 commented Apr 10, 2024

cpoile left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2024 •

edited

Loading