Refactor events listeners and fix a race condition when closing RDP stream by gzdunek · Pull Request #52714 · gravitational/teleport

gzdunek · 2025-03-03T17:42:52Z

I included a few changes in this PR:

Replaced listening to events from a verbose useEffect to a shorter useListener. The only point of this change is to limit repeating the same code, there's no functional changes.
Fixed a race condition that caused TDP errors to be overwritten with websocket close messages. This consists of two parts:
- On the backend side, a graceful disconnection now emits an error event instead of info event (I followed Zac's comment). Alternatively, we could convert TDP_INFO events to something like TDP_GRACEFUL_DISCONNECT, but I don't think we need it. In the UI we show a generic error in any case.
- On the frontend side, the socket.onclose() handler is removed before closing the socket when an error occurs first. Thanks to it, we don't overwrite the original error which is more important.
- Users should now consistently see this error when they click sign out in the remote machine:
I moved some functions from useTdpClientCanvas to the main component. The hook will be removed in a future PR, it only adds more layers of code which make navigating through it more difficult. There's no functional changes.

changelog: Resolved an issue that could cause WebSocket errors to appear after the graceful shutdown of a desktop session

Best to review commit by commit.

…bsocket close messages

It's the responsibility of callers who created the listeners to clean them up. Otherwise, `onWsClose` callback won't be called when we call `shutdown()`.

gzdunek · 2025-03-03T17:55:39Z

web/packages/teleport/src/lib/tdp/client.ts

  // It's safe to call this multiple times, calls subsequent to the first call
  // will simply do nothing.
  shutdown(closeCode = WebsocketCloseCode.NORMAL) {
-    this.removeAllListeners();


I think this shouldn't have been here in the first place.
This class doesn't create event listeners for its own purposes—its callers do. Because of that, it shouldn't be responsible for clearing them; that should be left to the callers. This became a problem for onWsClose callback I added. It was removed before we closed the socket so the listeners were not notified about it.

Previously, it worked purely by accident. useTdpClientCanvas.clientOnWsClient wasn't wrapped in useCallback, so when the user clicked the Disconnect button in the UI, a re-render occurred, causing the callback to be re-created and reattached as a listener.

gzdunek · 2025-03-03T18:04:52Z

web/packages/teleport/src/DesktopSession/useTdpClientCanvas.tsx

+        ...prevState,
+        {
+          content: info,
+          severity: 'info',


TDP_INFO is no longer used to signal disconnection, so now I show info warnings when we receive it.
But I'm not sure if it makes any sense, we don't have any code that sends info level alerts?

The original intent was that the TDP "notification" message can be used for both fatal errors or non-fatal messages.

SeverityError means there was a fatal error and the proxy is about to close the connection.

SeverityInfo and SeverityWarning are for non-fatal messages that show up in the top right corner.

In reality, Isaiah used SeverityInfo to show connection close "errors" in a state that's not error, which is why there's nothing else that's using INFO level.

Maybe the correct solution would have been to treat these as two separate concepts:

a severity, which indicates to the UI how to render the message

whether or not this message is preceding a disconnect

That said, what we have here is good enough for me.

zmb3 · 2025-03-03T18:52:43Z

Changes look good to me. I'll try to test it a bit later before approving.

zmb3

Works well, thanks!

zmb3 · 2025-03-05T17:37:23Z

I'm pretty sure this fixes #38373.

gzdunek · 2025-03-06T13:15:20Z

I think so, I will close the linked issue.

public-teleport-github-review-bot · 2025-03-06T13:55:40Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v15	Failed
branch/v16	Failed
branch/v17	Failed

…tream (#52714) * Add `useListener` hook that makes subscribing to events easier * Expose simpler APIs for subscribing to events * Replace verbose `useEffect` listeners with `useListener` calls * Fix a race condition that caused TDP errors to be overwritten with websocket close messages * Move `clientOnTdpWarning` to `DesktopCanvas.tsx` * Move `clientOnTdpInfo` to `DesktopCanvas.tsx` * Move `clientOnTdpError` to `DesktopCanvas.tsx` * Move functions that change WS connection status to `DesktopCanvas.tsx` * Change order of functions * Do not remove listeners when shutting down the WebSocket It's the responsibility of callers who created the listeners to clean them up. Otherwise, `onWsClose` callback won't be called when we call `shutdown()`. * Fix types (cherry picked from commit 0a7d941)

…P stream (#52869) * Refactor client canvas (#52431) * Remove non-canvas related listeners from `TdpClientCanvas` * Simplify event handlers setup by using React handlers * Listen to canvas resize events instead of window ones * Expose imperative API to set pointer * Expose imperative API to render frames * Expose imperative API to change canvas resolution Also, we no longer need to set the canvas size manually; it is now handled by `objectFit: scale-down`. * Remove duplicate player shutdown * Expose imperative API to clear canvas * Remove unnecessary display size syncing We get the canvas resolution from the server, there doesn't seem to be a need to calculate it from the browser window size. * Fix focusing canvas It didn't work because it happened too early, before we even showed the MFA dialog which stole focus. This is a temporary fix, I will do it more elegantly in the future. * Minor code changes Remove component memoization, it didn't work anyway and doesn't seem needed. We don't have anything particularly expensive to render here from React's perspective. * Add a TODO for improving rendering functions (cherry picked from commit 671475f) * Refactor events listeners and fix a race condition when closing RDP stream (#52714) * Add `useListener` hook that makes subscribing to events easier * Expose simpler APIs for subscribing to events * Replace verbose `useEffect` listeners with `useListener` calls * Fix a race condition that caused TDP errors to be overwritten with websocket close messages * Move `clientOnTdpWarning` to `DesktopCanvas.tsx` * Move `clientOnTdpInfo` to `DesktopCanvas.tsx` * Move `clientOnTdpError` to `DesktopCanvas.tsx` * Move functions that change WS connection status to `DesktopCanvas.tsx` * Change order of functions * Do not remove listeners when shutting down the WebSocket It's the responsibility of callers who created the listeners to clean them up. Otherwise, `onWsClose` callback won't be called when we call `shutdown()`. * Fix types (cherry picked from commit 0a7d941) * Rename warnings to alerts * Remove desktop session snapshot tests

…P stream (#52866) * Refactor client canvas (#52431) * Remove non-canvas related listeners from `TdpClientCanvas` * Simplify event handlers setup by using React handlers * Listen to canvas resize events instead of window ones * Expose imperative API to set pointer * Expose imperative API to render frames * Expose imperative API to change canvas resolution Also, we no longer need to set the canvas size manually; it is now handled by `objectFit: scale-down`. * Remove duplicate player shutdown * Expose imperative API to clear canvas * Remove unnecessary display size syncing We get the canvas resolution from the server, there doesn't seem to be a need to calculate it from the browser window size. * Fix focusing canvas It didn't work because it happened too early, before we even showed the MFA dialog which stole focus. This is a temporary fix, I will do it more elegantly in the future. * Minor code changes Remove component memoization, it didn't work anyway and doesn't seem needed. We don't have anything particularly expensive to render here from React's perspective. * Add a TODO for improving rendering functions (cherry picked from commit 671475f) * Refactor events listeners and fix a race condition when closing RDP stream (#52714) * Add `useListener` hook that makes subscribing to events easier * Expose simpler APIs for subscribing to events * Replace verbose `useEffect` listeners with `useListener` calls * Fix a race condition that caused TDP errors to be overwritten with websocket close messages * Move `clientOnTdpWarning` to `DesktopCanvas.tsx` * Move `clientOnTdpInfo` to `DesktopCanvas.tsx` * Move `clientOnTdpError` to `DesktopCanvas.tsx` * Move functions that change WS connection status to `DesktopCanvas.tsx` * Change order of functions * Do not remove listeners when shutting down the WebSocket It's the responsibility of callers who created the listeners to clean them up. Otherwise, `onWsClose` callback won't be called when we call `shutdown()`. * Fix types (cherry picked from commit 0a7d941)

…P stream (gravitational#52868) * Refactor client canvas (gravitational#52431) * Remove non-canvas related listeners from `TdpClientCanvas` * Simplify event handlers setup by using React handlers * Listen to canvas resize events instead of window ones * Expose imperative API to set pointer * Expose imperative API to render frames * Expose imperative API to change canvas resolution Also, we no longer need to set the canvas size manually; it is now handled by `objectFit: scale-down`. * Remove duplicate player shutdown * Expose imperative API to clear canvas * Remove unnecessary display size syncing We get the canvas resolution from the server, there doesn't seem to be a need to calculate it from the browser window size. * Fix focusing canvas It didn't work because it happened too early, before we even showed the MFA dialog which stole focus. This is a temporary fix, I will do it more elegantly in the future. * Minor code changes Remove component memoization, it didn't work anyway and doesn't seem needed. We don't have anything particularly expensive to render here from React's perspective. * Add a TODO for improving rendering functions (cherry picked from commit 671475f) * Refactor events listeners and fix a race condition when closing RDP stream (gravitational#52714) * Add `useListener` hook that makes subscribing to events easier * Expose simpler APIs for subscribing to events * Replace verbose `useEffect` listeners with `useListener` calls * Fix a race condition that caused TDP errors to be overwritten with websocket close messages * Move `clientOnTdpWarning` to `DesktopCanvas.tsx` * Move `clientOnTdpInfo` to `DesktopCanvas.tsx` * Move `clientOnTdpError` to `DesktopCanvas.tsx` * Move functions that change WS connection status to `DesktopCanvas.tsx` * Change order of functions * Do not remove listeners when shutting down the WebSocket It's the responsibility of callers who created the listeners to clean them up. Otherwise, `onWsClose` callback won't be called when we call `shutdown()`. * Fix types (cherry picked from commit 0a7d941) * Rename warning to alerts * Prettier * Remove desktop session snapshot tests

gzdunek added 10 commits March 3, 2025 17:52

Add useListener hook that makes subscribing to events easier

4ac92f7

Expose simpler APIs for subscribing to events

f8874c9

Replace verbose useEffect listeners with useListener calls

c19b7b6

Fix a race condition that caused TDP errors to be overwritten with we…

9574b68

…bsocket close messages

Move clientOnTdpWarning to DesktopCanvas.tsx

40aa29b

Move clientOnTdpInfo to DesktopCanvas.tsx

b59f39f

Move clientOnTdpError to DesktopCanvas.tsx

488d022

Move functions that change WS connection status to DesktopCanvas.tsx

e0600c2

Change order of functions

67f9a87

Do not remove listeners when shutting down the WebSocket

fcd3f0f

It's the responsibility of callers who created the listeners to clean them up. Otherwise, `onWsClose` callback won't be called when we call `shutdown()`.

gzdunek added backport/branch/v15 backport/branch/v17 labels Mar 3, 2025

gzdunek force-pushed the gzdunek/refactor-events-listeners branch from d9b0a53 to fcd3f0f Compare March 3, 2025 17:52

gzdunek commented Mar 3, 2025

View reviewed changes

gzdunek marked this pull request as ready for review March 3, 2025 17:56

github-actions bot requested review from flyinghermit, probakowski and ravicious March 3, 2025 17:57

github-actions bot added desktop-access rdp size/sm ui labels Mar 3, 2025

gzdunek requested review from avatus and zmb3 and removed request for flyinghermit, probakowski and ravicious March 3, 2025 17:57

gzdunek commented Mar 3, 2025

View reviewed changes

zmb3 approved these changes Mar 4, 2025

View reviewed changes

avatus approved these changes Mar 5, 2025

View reviewed changes

Fix types

c7ede5d

gzdunek enabled auto-merge March 6, 2025 13:15

gzdunek added this pull request to the merge queue Mar 6, 2025

Merged via the queue into master with commit 0a7d941 Mar 6, 2025
46 checks passed

gzdunek deleted the gzdunek/refactor-events-listeners branch March 6, 2025 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor events listeners and fix a race condition when closing RDP stream#52714

Refactor events listeners and fix a race condition when closing RDP stream#52714
gzdunek merged 11 commits intomasterfrom
gzdunek/refactor-events-listeners

gzdunek commented Mar 3, 2025 •

edited

Loading

Uh oh!

gzdunek Mar 3, 2025

Uh oh!

gzdunek Mar 3, 2025

Uh oh!

zmb3 Mar 3, 2025 •

edited

Loading

Uh oh!

zmb3 commented Mar 3, 2025

Uh oh!

zmb3 left a comment

Uh oh!

zmb3 commented Mar 5, 2025

Uh oh!

gzdunek commented Mar 6, 2025

Uh oh!

Uh oh!

public-teleport-github-review-bot bot commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gzdunek commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzdunek Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

gzdunek Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

zmb3 Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmb3 commented Mar 3, 2025

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

zmb3 commented Mar 5, 2025

Uh oh!

gzdunek commented Mar 6, 2025

Uh oh!

Uh oh!

public-teleport-github-review-bot bot commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gzdunek commented Mar 3, 2025 •

edited

Loading

zmb3 Mar 3, 2025 •

edited

Loading