starred/1Remote

Fork 0

mirror of https://github.com/1Remote/1Remote.git synced 2026-04-25 13:36:03 +03:00

[PR #1008] [MERGED] Fix RDP client disposal race condition causing Microsoft Store version crashes (#937) #939

New issue

Closed

opened 2026-02-26 12:00:33 +03:00 by kerem · 0 comments

kerem commented

2026-02-26 12:00:33 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/1Remote/1Remote/pull/1008
Author: @Copilot
Created: 10/19/2025
Status: ✅ Merged
Merged: 10/19/2025
Merged by: @VShawn

Base: main ← Head: copilot/fix-issue-937-and-verify

📝 Commits (3)

fba9271 Initial plan
1e0acf5 Fix RDP client disposal race condition in Microsoft Store version
3bbedfa Use dedicated lock object instead of lock(this) for RDP disposal

📊 Changes

1 file changed (+25 additions, -21 deletions)

View changed files

📝 Ui/View/Host/ProtocolHosts/AxMsRdpClient09Host.xaml.cs (+25 -21)

📄 Description

Problem

Issue #937 reported that the Microsoft Store version 1.2.0 crashes after RDP connections, while the sideloaded package works correctly. Investigation revealed a critical race condition in the RDP client disposal logic that was exposed more frequently by the Microsoft Store version's threading characteristics.

Root Cause

The RdpClientDispose() method in AxMsRdpClient09Host.xaml.cs used asynchronous disposal via Execute.OnUIThread(), which allowed code execution to continue before the RDP client was fully disposed. This created several race conditions:

// Before fix - ASYNC disposal
Execute.OnUIThread(DisposeRdpClient);  // Schedules disposal asynchronously
// Code continues immediately, before disposal completes!

Race condition scenarios:

Reconnection race: ReConn() calls RdpClientDispose() then immediately attempts to create a new RDP connection before the old client is fully disposed
Disconnect callback race: Disconnect handler calls RdpClientDispose() then invokes OnClosed callback before disposal completes
Double disposal race: Multiple threads could attempt to dispose the client simultaneously without proper synchronization

The log message "RDP Host: _rdpClient.Disposed." was printing before actual disposal completed, masking the timing issue.

Solution

Changed to synchronous disposal to ensure the RDP client is fully disposed before any subsequent operations:

// After fix - SYNC disposal
Execute.OnUIThreadSync(DisposeRdpClient);  // Waits for disposal to complete
// Code continues only after disposal is done

Additionally, introduced a dedicated lock object _rdpClientDisposeLock to ensure thread-safe access during creation and disposal, replacing the lock(this) pattern which can lead to deadlocks.

Changes

Changed Execute.OnUIThread() to Execute.OnUIThreadSync() in RdpClientDispose() for synchronous disposal
Added private readonly object _rdpClientDisposeLock for thread-safe disposal
Updated DisposeRdpClient(), CreateRdpClient(), and OnScreenResolutionChanged() to use the dedicated lock

Impact

These minimal, surgical changes ensure:

RDP client is fully disposed before reconnection attempts or callbacks execute
Thread-safe disposal prevents concurrent access crashes
Microsoft Store version should have the same stability as the sideloaded version
No API or behavior changes for existing functionality

Testing

Due to the WPF/Windows-specific nature of this code, testing requires a Windows environment. Recommended test scenarios:

RDP reconnection (disconnect and reconnect)
Rapid connection/disconnection cycles
Alternate address switching
Verify no regressions in normal RDP operations

Fixes #937

Original prompt

https://github.com/1Remote/1Remote/issues/937，阅读其中的讨论和相关issue和代码版本，分析问题现象和原因，并尝试解决。

如果能够正确解决该问题，修复所有涉及到的代码，并在修复后检查确认修改前后功能要保持一致。

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/1Remote/1Remote/pull/1008 **Author:** [@Copilot](https://github.com/apps/copilot-swe-agent) **Created:** 10/19/2025 **Status:** ✅ Merged **Merged:** 10/19/2025 **Merged by:** [@VShawn](https://github.com/VShawn) **Base:** `main` ← **Head:** `copilot/fix-issue-937-and-verify` --- ### 📝 Commits (3) - [`fba9271`](https://github.com/1Remote/1Remote/commit/fba9271c7c61d5a67909fe2f1d49a213b4e01511) Initial plan - [`1e0acf5`](https://github.com/1Remote/1Remote/commit/1e0acf505d9f20d161d885702cf9d93fa0b3e3d1) Fix RDP client disposal race condition in Microsoft Store version - [`3bbedfa`](https://github.com/1Remote/1Remote/commit/3bbedfaf582fc1f7d01784f23f4d394601c2323f) Use dedicated lock object instead of lock(this) for RDP disposal ### 📊 Changes **1 file changed** (+25 additions, -21 deletions) <details> <summary>View changed files</summary> 📝 `Ui/View/Host/ProtocolHosts/AxMsRdpClient09Host.xaml.cs` (+25 -21) </details> ### 📄 Description ## Problem Issue #937 reported that the Microsoft Store version 1.2.0 crashes after RDP connections, while the sideloaded package works correctly. Investigation revealed a critical race condition in the RDP client disposal logic that was exposed more frequently by the Microsoft Store version's threading characteristics. ## Root Cause The `RdpClientDispose()` method in `AxMsRdpClient09Host.xaml.cs` used **asynchronous disposal** via `Execute.OnUIThread()`, which allowed code execution to continue before the RDP client was fully disposed. This created several race conditions: ```csharp // Before fix - ASYNC disposal Execute.OnUIThread(DisposeRdpClient); // Schedules disposal asynchronously // Code continues immediately, before disposal completes! ``` **Race condition scenarios:** 1. **Reconnection race**: `ReConn()` calls `RdpClientDispose()` then immediately attempts to create a new RDP connection before the old client is fully disposed 2. **Disconnect callback race**: Disconnect handler calls `RdpClientDispose()` then invokes `OnClosed` callback before disposal completes 3. **Double disposal race**: Multiple threads could attempt to dispose the client simultaneously without proper synchronization The log message "RDP Host: _rdpClient.Disposed." was printing **before** actual disposal completed, masking the timing issue. ## Solution Changed to **synchronous disposal** to ensure the RDP client is fully disposed before any subsequent operations: ```csharp // After fix - SYNC disposal Execute.OnUIThreadSync(DisposeRdpClient); // Waits for disposal to complete // Code continues only after disposal is done ``` Additionally, introduced a dedicated lock object `_rdpClientDisposeLock` to ensure thread-safe access during creation and disposal, replacing the `lock(this)` pattern which can lead to deadlocks. ## Changes - Changed `Execute.OnUIThread()` to `Execute.OnUIThreadSync()` in `RdpClientDispose()` for synchronous disposal - Added `private readonly object _rdpClientDisposeLock` for thread-safe disposal - Updated `DisposeRdpClient()`, `CreateRdpClient()`, and `OnScreenResolutionChanged()` to use the dedicated lock ## Impact These minimal, surgical changes ensure: - RDP client is fully disposed before reconnection attempts or callbacks execute - Thread-safe disposal prevents concurrent access crashes - Microsoft Store version should have the same stability as the sideloaded version - No API or behavior changes for existing functionality ## Testing Due to the WPF/Windows-specific nature of this code, testing requires a Windows environment. Recommended test scenarios: - RDP reconnection (disconnect and reconnect) - Rapid connection/disconnection cycles - Alternate address switching - Verify no regressions in normal RDP operations Fixes #937  <details> <summary>Original prompt</summary> > 1. https://github.com/1Remote/1Remote/issues/937，阅读其中的讨论和相关issue和代码版本，分析问题现象和原因，并尝试解决。 > 2. 如果能够正确解决该问题，修复所有涉及到的代码，并在修复后检查确认修改前后功能要保持一致。 </details>  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

kerem

2026-02-26 12:00:33 +03:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/1Remote#939

No description provided.

Rows
Columns