Skip to content
This repository was archived by the owner on Nov 8, 2022. It is now read-only.

Commit a49bb67

Browse files
nvoronchevnvoronchev
committed
Fix race conditions is case of threaded Iterate() call
1 parent b288288 commit a49bb67

File tree

6 files changed

+163
-122
lines changed

6 files changed

+163
-122
lines changed

‎src/Bus.cs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,13 @@ public static Bus Starter
5959
throw new ArgumentNullException ("address");
6060

6161
Bus bus;
62-
if (buses.TryGetValue (address, out bus))
63-
return bus;
62+
lock (buses) {
63+
if (buses.TryGetValue (address, out bus))
64+
return bus;
6465

65-
bus = new Bus (address);
66-
buses[address] = bus;
66+
bus = new Bus (address);
67+
buses[address] = bus;
68+
}
6769

6870
return bus;
6971
}

‎src/BusException.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ public override string Message
2525
{
2626
get
2727
{
28-
return ErrorName + ": " + ErrorMessage;
28+
return ErrorName + ": " + ErrorMessage + Environment.NewLine + this.StackTrace;
2929
}
3030
}
3131

‎src/Connection.cs

Lines changed: 87 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
// See COPYING for details
44

55
using System;
6+
using System.Collections.Concurrent;
67
using System.Collections.Generic;
78
using System.IO;
89
using System.Threading;
10+
using System.Threading.Tasks;
911
using System.Reflection;
1012

1113
namespace DBus
@@ -37,23 +39,24 @@ public class Connection
3739

3840
Dictionary<uint,PendingCall> pendingCalls = new Dictionary<uint,PendingCall> ();
3941
Queue<Message> inbound = new Queue<Message> ();
40-
Dictionary<ObjectPath,BusObject> registeredObjects = new Dictionary<ObjectPath,BusObject> ();
42+
ConcurrentDictionary<ObjectPath,BusObject> registeredObjects = new ConcurrentDictionary<ObjectPath,BusObject> ();
43+
private readonly ReadMessageTask readMessageTask;
4144

4245
public delegate void MonitorEventHandler (Message msg);
4346
public MonitorEventHandler Monitors; // subscribe yourself to this list of observers if you want to get notified about each incoming message
4447

4548
protected Connection ()
4649
{
47-
50+
readMessageTask = new ReadMessageTask (this);
4851
}
4952

50-
internal Connection (Transport transport)
53+
internal Connection (Transport transport) : this ()
5154
{
5255
this.transport = transport;
5356
transport.Connection = this;
5457
}
5558

56-
internal Connection (string address)
59+
internal Connection (string address) : this ()
5760
{
5861
OpenPrivate (address);
5962
Authenticate ();
@@ -183,11 +186,12 @@ internal uint GenerateSerial ()
183186

184187
internal Message SendWithReplyAndBlock (Message msg, bool keepFDs)
185188
{
186-
PendingCall pending = SendWithReply (msg, keepFDs);
187-
return pending.Reply;
189+
using (PendingCall pending = SendWithPendingReply (msg, keepFDs)) {
190+
return pending.Reply;
191+
}
188192
}
189193

190-
internal PendingCall SendWithReply (Message msg, bool keepFDs)
194+
internal PendingCall SendWithPendingReply (Message msg, bool keepFDs)
191195
{
192196
msg.ReplyExpected = true;
193197

@@ -215,27 +219,23 @@ internal virtual uint Send (Message msg)
215219
return msg.Header.Serial;
216220
}
217221

218-
//temporary hack
219-
internal void DispatchSignals ()
222+
public void Iterate ()
220223
{
221-
lock (inbound) {
222-
while (inbound.Count != 0) {
223-
Message msg = inbound.Dequeue ();
224-
try {
225-
HandleSignal (msg);
226-
} finally {
227-
msg.Dispose ();
228-
}
229-
}
230-
}
224+
Iterate (new CancellationToken (false));
231225
}
232226

233-
public void Iterate ()
227+
public void Iterate (CancellationToken stopWaitToken)
234228
{
235-
Message msg = transport.ReadMessage ();
236-
237-
HandleMessage (msg);
238-
DispatchSignals ();
229+
if (TryGetStoredSignalMessage (out Message inboundMsg)) {
230+
try {
231+
HandleSignal (inboundMsg);
232+
} finally {
233+
inboundMsg.Dispose ();
234+
}
235+
} else {
236+
var msg = readMessageTask.MakeSureTaskRunAndWait (stopWaitToken);
237+
HandleMessage (msg);
238+
}
239239
}
240240

241241
internal virtual void HandleMessage (Message msg)
@@ -251,21 +251,19 @@ internal virtual void HandleMessage (Message msg)
251251
try {
252252

253253
//TODO: Restrict messages to Local ObjectPath?
254-
255254
{
256-
object field_value = msg.Header[FieldCode.ReplySerial];
255+
object field_value = msg.Header [FieldCode.ReplySerial];
257256
if (field_value != null) {
258257
uint reply_serial = (uint)field_value;
259-
PendingCall pending;
260258

261259
lock (pendingCalls) {
260+
PendingCall pending;
262261
if (pendingCalls.TryGetValue (reply_serial, out pending)) {
263-
if (pendingCalls.Remove (reply_serial)) {
264-
pending.Reply = msg;
265-
if (pending.KeepFDs)
266-
cleanupFDs = false; // caller is responsible for closing FDs
267-
}
268-
262+
if (!pendingCalls.Remove (reply_serial))
263+
return;
264+
pending.Reply = msg;
265+
if (pending.KeepFDs)
266+
cleanupFDs = false; // caller is responsible for closing FDs
269267
return;
270268
}
271269
}
@@ -285,8 +283,7 @@ internal virtual void HandleMessage (Message msg)
285283
break;
286284
case MessageType.Signal:
287285
//HandleSignal (msg);
288-
lock (inbound)
289-
inbound.Enqueue (msg);
286+
StoreInboundSignalMessage (msg); //temporary hack
290287
cleanupFDs = false; // FDs are closed after signal is handled
291288
break;
292289
case MessageType.Error:
@@ -391,14 +388,15 @@ internal void HandleMethodCall (MessageContainer method_call)
391388
//this is messy and inefficient
392389
List<string> linkNodes = new List<string> ();
393390
int depth = method_call.Path.Decomposed.Length;
394-
foreach (ObjectPath pth in registeredObjects.Keys) {
395-
if (pth.Value == (method_call.Path.Value)) {
396-
ExportObject exo = (ExportObject)registeredObjects[pth];
391+
foreach(var objKeyValuePair in registeredObjects) {
392+
ObjectPath pth = objKeyValuePair.Key;
393+
ExportObject exo = (ExportObject) objKeyValuePair.Value;
394+
if (pth.Value == method_call.Path.Value) {
397395
exo.WriteIntrospect (intro);
398396
} else {
399-
for (ObjectPath cur = pth ; cur != null ; cur = cur.Parent) {
397+
for (ObjectPath cur = pth; cur != null; cur = cur.Parent) {
400398
if (cur.Value == method_call.Path.Value) {
401-
string linkNode = pth.Decomposed[depth];
399+
string linkNode = pth.Decomposed [depth];
402400
if (!linkNodes.Contains (linkNode)) {
403401
intro.WriteNode (linkNode);
404402
linkNodes.Add (linkNode);
@@ -415,9 +413,8 @@ internal void HandleMethodCall (MessageContainer method_call)
415413
return;
416414
}
417415

418-
BusObject bo;
419-
if (registeredObjects.TryGetValue (method_call.Path, out bo)) {
420-
ExportObject eo = (ExportObject)bo;
416+
if (registeredObjects.TryGetValue(method_call.Path, out BusObject bo)) {
417+
ExportObject eo = (ExportObject) bo;
421418
eo.HandleMethodCall (method_call);
422419
} else {
423420
MaybeSendUnknownMethodError (method_call);
@@ -464,14 +461,10 @@ public void Register (ObjectPath path, object obj)
464461

465462
public object Unregister (ObjectPath path)
466463
{
467-
BusObject bo;
468-
469-
if (!registeredObjects.TryGetValue (path, out bo))
464+
if (!registeredObjects.TryRemove (path, out BusObject bo))
470465
throw new Exception ("Cannot unregister " + path + " as it isn't registered");
471466

472-
registeredObjects.Remove (path);
473-
474-
ExportObject eo = (ExportObject)bo;
467+
ExportObject eo = (ExportObject) bo;
475468
eo.Registered = false;
476469

477470
return eo.Object;
@@ -486,6 +479,25 @@ internal protected virtual void RemoveMatch (string rule)
486479
{
487480
}
488481

482+
private void StoreInboundSignalMessage (Message msg)
483+
{
484+
lock (inbound) {
485+
inbound.Enqueue (msg);
486+
}
487+
}
488+
489+
private bool TryGetStoredSignalMessage (out Message msg)
490+
{
491+
msg = null;
492+
lock (inbound) {
493+
if (inbound.Count != 0) {
494+
msg = inbound.Dequeue ();
495+
return true;
496+
}
497+
}
498+
return false;
499+
}
500+
489501
static UUID ReadMachineId (string fname)
490502
{
491503
byte[] data = File.ReadAllBytes (fname);
@@ -494,5 +506,31 @@ static UUID ReadMachineId (string fname)
494506

495507
return UUID.Parse (System.Text.Encoding.ASCII.GetString (data, 0, 32));
496508
}
509+
510+
private class ReadMessageTask
511+
{
512+
private readonly Connection ownerConnection;
513+
private Task<Message> task = null;
514+
private object taskLock = new object ();
515+
516+
public ReadMessageTask (Connection connection)
517+
{
518+
ownerConnection = connection;
519+
}
520+
521+
public Message MakeSureTaskRunAndWait(CancellationToken stopWaitToken)
522+
{
523+
Task<Message> catchedTask = null;
524+
525+
lock (taskLock) {
526+
if (task == null || task.IsCompleted) {
527+
task = Task<Message>.Run (() => ownerConnection.transport.ReadMessage ());
528+
}
529+
catchedTask = task;
530+
}
531+
catchedTask.Wait (stopWaitToken);
532+
return catchedTask.Result;
533+
}
534+
}
497535
}
498536
}

‎src/ExportObject.cs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,11 @@ internal virtual void WriteIntrospect (Introspector intro)
6767
internal static MethodCaller GetMCaller (MethodInfo mi)
6868
{
6969
MethodCaller mCaller;
70-
if (!mCallers.TryGetValue (mi, out mCaller)) {
71-
mCaller = TypeImplementer.GenCaller (mi);
72-
mCallers[mi] = mCaller;
70+
lock (mCallers) {
71+
if (!mCallers.TryGetValue (mi, out mCaller)) {
72+
mCaller = TypeImplementer.GenCaller (mi);
73+
mCallers[mi] = mCaller;
74+
}
7375
}
7476
return mCaller;
7577
}
@@ -90,11 +92,7 @@ public virtual void HandleMethodCall (MessageContainer method_call)
9092
return;
9193
}
9294

93-
MethodCaller mCaller;
94-
if (!mCallers.TryGetValue (mi, out mCaller)) {
95-
mCaller = TypeImplementer.GenCaller (mi);
96-
mCallers[mi] = mCaller;
97-
}
95+
MethodCaller mCaller = GetMCaller (mi);
9896

9997
Signature inSig, outSig;
10098
bool hasDisposableList;

‎src/Protocol/PendingCall.cs

Lines changed: 29 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -7,23 +7,33 @@
77

88
namespace DBus.Protocol
99
{
10-
public class PendingCall : IAsyncResult
10+
public class PendingCall : IAsyncResult, IDisposable
1111
{
12-
Connection conn;
13-
Message reply;
14-
ManualResetEvent waitHandle;
15-
bool completedSync;
16-
bool keepFDs;
17-
12+
private Connection conn;
13+
private Message reply;
14+
private ManualResetEvent waitHandle = new ManualResetEvent (false);
15+
private bool completedSync = false;
16+
private bool keepFDs;
17+
private CancellationTokenSource stopWait = new CancellationTokenSource();
18+
1819
public event Action<Message> Completed;
1920

20-
public PendingCall (Connection conn) : this (conn, false) {}
21+
public PendingCall(Connection conn)
22+
: this (conn, false)
23+
{
24+
}
25+
2126
public PendingCall (Connection conn, bool keepFDs)
2227
{
2328
this.conn = conn;
2429
this.keepFDs = keepFDs;
2530
}
2631

32+
public void Dispose()
33+
{
34+
stopWait.Dispose ();
35+
}
36+
2737
internal bool KeepFDs
2838
{
2939
get {
@@ -33,36 +43,24 @@ internal bool KeepFDs
3343

3444
public Message Reply {
3545
get {
36-
if (reply != null)
37-
return reply;
38-
39-
if (Thread.CurrentThread == conn.mainThread) {
40-
while (reply == null)
41-
conn.HandleMessage (conn.Transport.ReadMessage ());
42-
43-
completedSync = true;
44-
45-
conn.DispatchSignals ();
46-
} else {
47-
if (waitHandle == null)
48-
Interlocked.CompareExchange (ref waitHandle, new ManualResetEvent (false), null);
49-
50-
while (reply == null)
51-
waitHandle.WaitOne ();
52-
53-
completedSync = false;
46+
while (reply == null) {
47+
try {
48+
conn.Iterate (stopWait.Token);
49+
}
50+
catch (OperationCanceledException) {
51+
}
5452
}
55-
5653
return reply;
57-
}
54+
}
55+
5856
set {
5957
if (reply != null)
6058
throw new Exception ("Cannot handle reply more than once");
61-
6259
reply = value;
6360

64-
if (waitHandle != null)
65-
waitHandle.Set ();
61+
waitHandle.Set ();
62+
63+
stopWait.Cancel ();
6664

6765
if (Completed != null)
6866
Completed (reply);
@@ -84,9 +82,6 @@ object IAsyncResult.AsyncState {
8482

8583
WaitHandle IAsyncResult.AsyncWaitHandle {
8684
get {
87-
if (waitHandle == null)
88-
waitHandle = new ManualResetEvent (false);
89-
9085
return waitHandle;
9186
}
9287
}

0 commit comments

Comments
 (0)