Skip to content

Commit 281e01e

Browse files
authored
Merge pull request #267 from mono/vsts-1026591
Use a different lock to manage breakpointStore
2 parents 1f4b360 + eb2d221 commit 281e01e

File tree

3 files changed

+209
-20
lines changed

3 files changed

+209
-20
lines changed

Mono.Debugging/Mono.Debugging.Client/DebuggerSession.cs

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ public abstract class DebuggerSession: IDisposable
5252
readonly InternalDebuggerSession frontend;
5353
readonly object slock = new object ();
5454
readonly EvaluationStatistics evaluationStats = new EvaluationStatistics ();
55+
readonly object breakpointStoreLock = new object ();
5556
BreakpointStore breakpointStore;
5657
DebuggerSessionOptions options;
5758
ProcessInfo[] currentProcesses;
@@ -225,7 +226,7 @@ public EvaluationStatistics EvaluationStats {
225226
/// </summary>
226227
public BreakpointStore Breakpoints {
227228
get {
228-
lock (slock) {
229+
lock (breakpointStoreLock) {
229230
if (breakpointStore == null) {
230231
Breakpoints = new BreakpointStore ();
231232
ownedBreakpointStore = true;
@@ -234,14 +235,13 @@ public BreakpointStore Breakpoints {
234235
}
235236
}
236237
set {
237-
lock (slock) {
238+
lock (breakpointStoreLock) {
238239
if (breakpointStore != null) {
239-
lock (breakpointStore) {
240-
foreach (BreakEvent bp in breakpointStore) {
241-
RemoveBreakEvent (bp);
242-
NotifyBreakEventStatusChanged (bp);
243-
}
240+
foreach (BreakEvent bp in breakpointStore) {
241+
RemoveBreakEvent (bp);
242+
NotifyBreakEventStatusChanged (bp);
244243
}
244+
245245
breakpointStore.BreakEventAdded -= OnBreakpointAdded;
246246
breakpointStore.BreakEventRemoved -= OnBreakpointRemoved;
247247
breakpointStore.BreakEventModified -= OnBreakpointModified;
@@ -254,21 +254,20 @@ public BreakpointStore Breakpoints {
254254
ownedBreakpointStore = false;
255255

256256
if (breakpointStore != null) {
257+
breakpointStore.BreakEventAdded += OnBreakpointAdded;
258+
breakpointStore.BreakEventRemoved += OnBreakpointRemoved;
259+
breakpointStore.BreakEventModified += OnBreakpointModified;
260+
breakpointStore.BreakEventEnableStatusChanged += OnBreakpointStatusChanged;
261+
breakpointStore.CheckingReadOnly += BreakpointStoreCheckingReadOnly;
262+
257263
if (IsConnected) {
258264
Dispatch (delegate {
259265
if (IsConnected) {
260-
lock (breakpointStore) {
261-
foreach (BreakEvent bp in breakpointStore)
262-
AddBreakEvent (bp);
263-
}
266+
foreach (BreakEvent bp in breakpointStore)
267+
AddBreakEvent (bp);
264268
}
265269
});
266270
}
267-
breakpointStore.BreakEventAdded += OnBreakpointAdded;
268-
breakpointStore.BreakEventRemoved += OnBreakpointRemoved;
269-
breakpointStore.BreakEventModified += OnBreakpointModified;
270-
breakpointStore.BreakEventEnableStatusChanged += OnBreakpointStatusChanged;
271-
breakpointStore.CheckingReadOnly += BreakpointStoreCheckingReadOnly;
272271
}
273272
}
274273
}
@@ -1156,10 +1155,8 @@ internal protected virtual void OnStarted (ThreadInfo t)
11561155
if (!HasExited) {
11571156
IsConnected = true;
11581157
if (breakpointStore != null) {
1159-
lock (breakpointStore) {
1160-
foreach (BreakEvent bp in breakpointStore)
1161-
AddBreakEvent (bp);
1162-
}
1158+
foreach (BreakEvent bp in breakpointStore)
1159+
AddBreakEvent (bp);
11631160
}
11641161
}
11651162
}

UnitTests/Mono.Debugging.Tests/Shared/DebugTests.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,8 @@ protected virtual void Start (string test)
154154
{
155155
TestName = test;
156156
Session = CreateSession (test, EngineId);
157+
// make sure we have a breakpoint store created and setup
158+
Session.Breakpoints.Clear ();
157159

158160
var dsi = CreateStartInfo (test, EngineId);
159161
var soft = dsi as SoftDebuggerStartInfo;
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
//
2+
// DebuggerSessionTests.cs
3+
//
4+
// Author:
5+
// Greg Munn <gregm@microsoft.com>
6+
//
7+
// Copyright (c) 2019 Microsoft
8+
//
9+
// Permission is hereby granted, free of charge, to any person obtaining a copy
10+
// of this software and associated documentation files (the "Software"), to deal
11+
// in the Software without restriction, including without limitation the rights
12+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
13+
// copies of the Software, and to permit persons to whom the Software is
14+
// furnished to do so, subject to the following conditions:
15+
//
16+
// The above copyright notice and this permission notice shall be included in
17+
// all copies or substantial portions of the Software.
18+
//
19+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
20+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
21+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
22+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
23+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
24+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
25+
// THE SOFTWARE.
26+
27+
using System;
28+
using System.Threading;
29+
using System.Threading.Tasks;
30+
31+
using Mono.Debugging.Client;
32+
33+
using NUnit.Framework;
34+
35+
namespace Mono.Debugging.Tests
36+
{
37+
[TestFixture]
38+
public class DebuggerSessionTests
39+
{
40+
TestDebuggerSession session;
41+
42+
[TestFixtureSetUp]
43+
public virtual void SetUp ()
44+
{
45+
session = new TestDebuggerSession ();
46+
}
47+
48+
[TestFixtureTearDown]
49+
public virtual void TearDown ()
50+
{
51+
session.Dispose ();
52+
}
53+
54+
[Test]
55+
#pragma warning disable VSTHRD200 // Use "Async" suffix for async methods
56+
public async Task WhenQueryigBreakpoints_ThenDoNotDeadlockDispatchedMethods()
57+
#pragma warning restore VSTHRD200 // Use "Async" suffix for async methods
58+
{
59+
// thread 1 cancels debugging, which triggers hot reload to clear breakpoints
60+
// thread 2 dispatches the OnExit method which is call via Dispatch and uses the same lock
61+
62+
var thread1 = new TaskCompletionSource<bool> ();
63+
Task thread2 = null;
64+
65+
session.HandleOnExit = () => {
66+
// we are on a background thread via session.Dispatch
67+
thread2 = Task.Run (() => {
68+
var bk = session.Breakpoints;
69+
thread1.TrySetResult (true);
70+
});
71+
72+
// mimic the cause of the deadlock by waiting here for a while
73+
// we don't really want to deadlock, just long enough that we should have
74+
// completed the call to breakpoints first.
75+
Thread.Sleep (1000);
76+
77+
thread1.TrySetResult (false);
78+
};
79+
80+
session.Exit ();
81+
82+
var result= await thread1.Task;
83+
await thread2;
84+
85+
Assert.IsTrue (result, "the call to get breakpoints should have completed first");
86+
}
87+
88+
class TestDebuggerSession : DebuggerSession
89+
{
90+
public Action HandleOnExit;
91+
92+
protected override void OnAttachToProcess (long processId)
93+
{
94+
throw new NotImplementedException ();
95+
}
96+
97+
protected override void OnContinue ()
98+
{
99+
throw new NotImplementedException ();
100+
}
101+
102+
protected override void OnDetach ()
103+
{
104+
throw new NotImplementedException ();
105+
}
106+
107+
protected override void OnEnableBreakEvent (BreakEventInfo eventInfo, bool enable)
108+
{
109+
throw new NotImplementedException ();
110+
}
111+
112+
protected override void OnExit ()
113+
{
114+
// this is called in dispatch
115+
HandleOnExit?.Invoke ();
116+
//throw new NotImplementedException ();
117+
}
118+
119+
protected override void OnFinish ()
120+
{
121+
throw new NotImplementedException ();
122+
}
123+
124+
protected override ProcessInfo [] OnGetProcesses ()
125+
{
126+
throw new NotImplementedException ();
127+
}
128+
129+
protected override Backtrace OnGetThreadBacktrace (long processId, long threadId)
130+
{
131+
throw new NotImplementedException ();
132+
}
133+
134+
protected override ThreadInfo [] OnGetThreads (long processId)
135+
{
136+
throw new NotImplementedException ();
137+
}
138+
139+
protected override BreakEventInfo OnInsertBreakEvent (BreakEvent breakEvent)
140+
{
141+
throw new NotImplementedException ();
142+
}
143+
144+
protected override void OnNextInstruction ()
145+
{
146+
throw new NotImplementedException ();
147+
}
148+
149+
protected override void OnNextLine ()
150+
{
151+
throw new NotImplementedException ();
152+
}
153+
154+
protected override void OnRemoveBreakEvent (BreakEventInfo eventInfo)
155+
{
156+
throw new NotImplementedException ();
157+
}
158+
159+
protected override void OnRun (DebuggerStartInfo startInfo)
160+
{
161+
throw new NotImplementedException ();
162+
}
163+
164+
protected override void OnSetActiveThread (long processId, long threadId)
165+
{
166+
throw new NotImplementedException ();
167+
}
168+
169+
protected override void OnStepInstruction ()
170+
{
171+
throw new NotImplementedException ();
172+
}
173+
174+
protected override void OnStepLine ()
175+
{
176+
throw new NotImplementedException ();
177+
}
178+
179+
protected override void OnStop ()
180+
{
181+
throw new NotImplementedException ();
182+
}
183+
184+
protected override void OnUpdateBreakEvent (BreakEventInfo eventInfo)
185+
{
186+
throw new NotImplementedException ();
187+
}
188+
}
189+
}
190+
}

0 commit comments

Comments
 (0)