microsoft / WindowsDevicePortalWrapper

A client library that wraps the Windows Device Portal REST APIs.
MIT License
182 stars 87 forks source link

Deadlock Using Start/StopListeningForSystemPerf defined in PeformaceData.cs #150

Closed GregPettyjohn closed 8 years ago

GregPettyjohn commented 8 years ago

As per Matt: "[‎8/‎25/‎2016 2:28 PM] Matt Hyman: Very easy to deadlock on web socket on .net, receive should be on a worker thread with a timeout on closing the web socket"

TL;DR Here are the details with a enough breadcrumbs so that I can reconstruct a repro in my sample:

Ran into a deadlock in my MVVM WPF application when using the using the WebSocket perf listener. My ViewModel has the following two async methods that I use as delegates for an ICommand:

private async Task ExecuteStartListeningForSystemPerfAsync() { this.Ready = false; this.portal.SystemPerfMessageReceived += OnSystemPerfReceived; await this.portal.StartListeningForSystemPerf(); this.Ready = true; }

private async Task ExecuteStopListeningForSystemPerfAsync() { this.Ready = false; this.portal.SystemPerfMessageReceived -= OnSystemPerfReceived; await this.portal.StopListeningForSystemPerf(); this.Ready = true; }

Each is supplied to DelegateCommand.FromAsyncHandler(...) where DelegateCommand is implemented in the PRISM library.

ExecuteStopListeningForSystemPerfAsycn() causes a deadlock on the Main thread. In the debugger, we see that StopListeningForMessagesInternal [WebSocket.cs] never returns from the WaitOne() call:

private async Task StopListeningForMessagesInternal() { if (this.IsListeningForMessages) { await this.websocket.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, string.Empty, CancellationToken.None);

            // Wait for web socket to no longer be receiving messages.
            if (this.IsListeningForMessages)
            {
                this.stoppedReceivingMessages.WaitOne();
                this.stoppedReceivingMessages.Reset();
            }

            // Reset websocket as WDP will abort the connection once it receives the close message.
            this.websocket = new ClientWebSocket();
        }
    }

The way to avoid the deadlock was to use Task.Run() to force the call to occur on a pool thread.

WilliamsJason commented 8 years ago

It looks like these are marked as async methods but they don't actually run asynchronously (I'm seeing a warning in my solution for them). Could be they shouldn't be async, which might resolve some of the problems?

MattHyman commented 8 years ago

Resolved with PR #162