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 TDP/RDP termination #13912

Merged
merged 5 commits into from
Jun 30, 2022
Merged

Fix TDP/RDP termination #13912

merged 5 commits into from
Jun 30, 2022

Conversation

ibeckermayer
Copy link
Contributor

@ibeckermayer ibeckermayer commented Jun 27, 2022

In #13479 I cleaned up the Client's API and centralized the connection and memory cleanup, but I also removed a deferred c.Close() from the streaming goroutines. This change meant that in the most common case of the TDP input streaming goroutine erroring due to the browser tab being closed:

msg, err := c.cfg.Conn.InputMessage()
if errors.Is(err, io.EOF) {
return

the RDP streaming goroutine was no longer being terminated immediately, causing it to awkwardly hang around failing calls until the idle timeout (possibly relates to #13723).

This change reintroduces the deferred c.close() to streaming goroutines and also adds insurance that the TDP connection is closed in c.close(), so that whenever one goroutine terminates, the other one isn't left far behind.

…rance that the tdp connection is closed to c.close()
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

I'm also curious how this behaves if windows terminates the connection - do we see any confusing warnings or errors in the console?

(test by killing/rebooting a windows VM during a session)

@ibeckermayer
Copy link
Contributor Author

I'm also curious how this behaves if windows terminates the connection - do we see any confusing warnings or errors in the console?

(test by killing/rebooting a windows VM during a session)

Good question -- the screen just freezes for a while (which is sorta confusing), until the rust rdp client times out here

let res = client.rdp_client.lock().unwrap().read(|rdp_event| {
(takes about 20sec or so).

Nothing too crazy looking in the logs:

2022-06-27T16:58:05-04:00 DEBU [PROC:1]    Broadcasting event. event:TeleportDegraded pid:11293.1 service/supervisor.go:376
[2022-06-27T20:58:07Z ERROR rdp_client] RDP read failed: Io(Os { code: 60, kind: TimedOut, message: "Operation timed out" })
2022-06-27T16:58:07-04:00 WARN [WINDOWS_D] Failed reading RDP output frame: 1 client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 rdp-addr:172.16.97.147:3389 rdpclient/client.go:265
2022-06-27T16:58:07-04:00 WARN [WINDOWS_D] Failed reading TDP input message: read tcp [::1]:3028->[::1]:56465: use of closed network connection client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 rdp-addr:172.16.97.147:3389 rdpclient/client.go:289
2022-06-27T16:58:07-04:00 INFO [WINDOWS_D] TDP input streaming finished client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 rdp-addr:172.16.97.147:3389 rdpclient/client.go:290
[2022-06-27T20:58:07Z DEBUG rustls::conn] Sending warning alert CloseNotify
[2022-06-27T20:58:07Z ERROR rdp_client] failed writing RDP keyboard event: Io(Os { code: 32, kind: BrokenPipe, message: "Broken pipe" })
2022-06-27T16:58:07-04:00 DEBU [SSH:PROXY] Closed connection [::1]:56466. sshutils/server.go:449
2022-06-27T16:58:07-04:00 INFO [WINDOWS_D] RDP output streaming finished client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 rdp-addr:172.16.97.147:3389 rdpclient/client.go:273
2022-06-27T16:58:07-04:00 WARN [WINDOWS_D] error closing the RDP connection client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 rdp-addr:172.16.97.147:3389 rdpclient/client.go:529
2022-06-27T16:58:07-04:00 DEBU [WINDOWS_D] Windows desktop disconnected client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 desktop/windows_server.go:758
2022-06-27T16:58:07-04:00 DEBU [WINDOWS_D] Releasing associated resources - context has been closed. client-ip:::1 desktop-addr:172.16.97.147:3389 desktop-name:WIN-1E6JDI6PHQA-teleport-dev pid:11293.1 srv/monitor.go:253

@zmb3

@ibeckermayer ibeckermayer enabled auto-merge (squash) June 27, 2022 22:58
@ibeckermayer ibeckermayer requested a review from zmb3 June 30, 2022 18:39
@ibeckermayer
Copy link
Contributor Author

@nklaassen @LKozlowski PTAL when you next have a chance

@nklaassen nklaassen disabled auto-merge June 30, 2022 19:41
@nklaassen nklaassen enabled auto-merge (squash) June 30, 2022 19:41
@nklaassen nklaassen disabled auto-merge June 30, 2022 19:42
@nklaassen nklaassen enabled auto-merge (squash) June 30, 2022 19:42
@nklaassen
Copy link
Contributor

sorry, misclicked on mobile.. will review

Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

LGTM

@nklaassen nklaassen merged commit 6f32794 into master Jun 30, 2022
@github-actions
Copy link

@ibeckermayer See the table below for backport results.

Branch Result
branch/v10 Create PR
branch/v9 Create PR

@zmb3 zmb3 deleted the isaiah/fix-tdp-rdp-streaming-term branch July 22, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants