Firing events from within a critical section

[Guest]

Hi,

I have a component where I need to have thread-safe access to a list.
Operations on the list are done in critical sections (lock (lockObject) { ...
} ) in the usual way, to make sure that no other thread will modify the list
while operations are running.

However, I at the end of the critical section, I need to fire an OnChange
type of event, publishing the changed list item as part of my custom
EventArgs. I want to be certain that by the time the event reaches
subscribers, another thread (that will have locked the list in its critical
section) won't have changed the item.

Is it safe to invoke OnChange (in the safe usual way) from within the
critical section? Wouldn't that introduce potential reentrancy issues? An
alternative could be to make a deep clone of the changed Item, but wouldn't
that be expensive for larger class (the list is an IList<T>, and T could be
pretty large to clone).

So, to recap:

lock (lockObject)
{
T Item = List[3];
// do some work with Item
SafelyFireOnChange(this, new MyEventArgs(Item))
}

or

lock (lockObject)
{
T Item = List[3];
// do some work with Item
}
// ...Item might change at this point in the execution...
SafelyFireOnChange(this, new MyEventArgs(Item))

Thanks, regards,
Vanni

 

[Nicholas Paldino [.NET/C# MVP]]

Vanni,

There is nothing stopping you from firing the event in the lock section.
However, you are opening yourself up to potential issues if lockObject is
exposed to the event handlers. The issues will come into play if you spawn
new threads and then use lockObject in lock statements in those threads (you
could possibly deadlock if the calling thread waits on the other threads
that are spawned).

 

[Guest]

Hi Nicholas, thanks for your prompt reply.
lockObject is obviously a private object visible, not exposed in the event
handler.

Regards,
Vanni

Vanni,

There is nothing stopping you from firing the event in the lock section.
However, you are opening yourself up to potential issues if lockObject is
exposed to the event handlers. The issues will come into play if you spawn
new threads and then use lockObject in lock statements in those threads (you
could possibly deadlock if the calling thread waits on the other threads
that are spawned).

--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

Hi,

I have a component where I need to have thread-safe access to a list.
Operations on the list are done in critical sections (lock (lockObject)
{ ...
} ) in the usual way, to make sure that no other thread will modify the
list
while operations are running.

However, I at the end of the critical section, I need to fire an OnChange
type of event, publishing the changed list item as part of my custom
EventArgs. I want to be certain that by the time the event reaches
subscribers, another thread (that will have locked the list in its
critical
section) won't have changed the item.

Is it safe to invoke OnChange (in the safe usual way) from within the
critical section? Wouldn't that introduce potential reentrancy issues? An
alternative could be to make a deep clone of the changed Item, but
wouldn't
that be expensive for larger class (the list is an IList<T>, and T could
be
pretty large to clone).

So, to recap:

lock (lockObject)
{
T Item = List[3];
// do some work with Item
SafelyFireOnChange(this, new MyEventArgs(Item))
}

or

lock (lockObject)
{
T Item = List[3];
// do some work with Item
}
// ...Item might change at this point in the execution...
SafelyFireOnChange(this, new MyEventArgs(Item))

Thanks, regards,
Vanni

 

[Peter Duniho]

Hi Nicholas, thanks for your prompt reply.
lockObject is obviously a private object visible, not exposed in the event
handler.

I'm not sure, however, that his comments are meant to be restricted to
situations in which the lockObject is explicitly exposed.

The mere act of calling other code does, in a way, expose the lockObject
implicitly. Other threads can't use it directly, but you've already
gotten a lock on it. A problem can occur if the event handler waits for
a different locked object held by another thread, while that other
thread is waiting for the locked lockObject.

You mentioned "potential reentrancy issues", which probably isn't a
problem. If by "reentrancy" you mean the usual "reentering the same
code", that's fine. You can lock the same object arbitrarily many times
from within the same thread. So if that's what you thought might be a
problem, you can rest easy there.

But that doesn't mean there's no potential for a problem.

By providing a way for arbitrary client code to be called (eg via an
event), you open the possibility for that arbitrary client code to cause
a deadlock, depending on what it does.

A classic example of a way this might break: you get the lock on some
worker thread, then raise an event; the event handler wants to update
the UI, so it calls Invoke(); on the UI thread, some operation is
performed that attempts to gain access to the resource locked via the
lockObject object.

Deadlock: there is already a lock on lockObject, so the UI thread can't
get it. But the lock will never be released because the thread that has
the lock is waiting on the UI thread.

Whether this applies in your situation is simply a matter of the design.
But the _potential_ for harm does exist. The key is to design your
code so that you don't realize that potential.

Now, all that said...your example appears to be locking to protect the
list, not the items within the list. So I don't actually see a problem
just passing the item itself to the event handler. Assuming the item
itself is already thread-safe, what you want to do seems fine. Though,
for that matter, it's not clear why locking the list in any way protects
the item itself anyway.

Because of the lock, the event handler (presumably) should be assured
that the list containing the item hasn't changed, but unless you use the
same lockObject to protect the items as well (and IMHO that wouldn't be
a good design), there's no similar assurance about the item. And of
course, it's the item that the event handler cares about.

So probably you can just put the variable that references the item
outside of the lock() statement, and raise the event after exiting the
lock().

Pete

 

[Guest]

Excellent elaboration Peter, thanks for that!

I can see resource locking poses more issues than it solves.

Perhaps I could make myself cleared by actually making an example in
(working) code.

public C this[Int32 index]
{
get
{
[ ...code omitted...]
}

set
{
C OldItem = m_Contents[index];
lock (m_Lock)
{
if (index < m_Contents.Count)
{
m_Contents[index] = value;
FireWriteEvent(OldItem,
(int)EventPublisherSpecifiers.ItemRemoved);
FireWriteEvent(value,
(int)EventPublisherSpecifiers.ItemAdded);
}
}
}
}


However generic I am trying to keep the design, the C parameter will be most
probably representing a key/value pair, exposed to continuous change by a
concurrent "updater" thread. Fine, that's how the application is supposed to
work. When, however, the list of these key/value pair is changed and the
event handler is (safely) fired up, I would want the subscribers to receive
the values (OldItem, "value", etc.) at the moment the list operation was
performed. In other words, I want the updater thread to be locked out while
the event handler is called.

Now, I understand that the tricky bit is that event handler being invoked on
the subscriber does NOT mean subscriber has successfully consumed those
objects (via custom EventArgs). The only two alternatives I can see to this
problems are:

1) Making a deep clone of the C object, so as to make sure that value is
disconnected from the list and cannot be changed by the updater thread. This
is perhaps the best solution, but introduces the overheads of cloning an
object every single time a notification is fired up. For a simple key/value
pair,
it shouldn't be too bad. Problems may arise when the C object represent a
bigger class.

2) Keeping the list locked until every subscriber has informed the class
that value has been consumed. This means the critical code section would wait
for objects held by every single subscriber, hence opening up a huge
potential for deadlock.


What do you think?

Thanks, regards,
Vanni

 

[Peter Duniho]

Excellent elaboration Peter, thanks for that!

You're welcome.

I can see resource locking poses more issues than it solves.

Perhaps I could make myself cleared by actually making an example in
(working) code.

public C this[Int32 index]
{
get
{
[ ...code omitted...]
}

set
{
C OldItem = m_Contents[index];
lock (m_Lock)
{
if (index < m_Contents.Count)
{
m_Contents[index] = value;
FireWriteEvent(OldItem,
(int)EventPublisherSpecifiers.ItemRemoved);
FireWriteEvent(value,
(int)EventPublisherSpecifiers.ItemAdded);
}
}
}
}

At a minimum, the above code seems wrong. That is, you get OldItem
before you lock the list. This means that you could be getting a member
of the list that's out-of-date or, worse, is only partially updated.
The exact scenario would depend, of course, on what type C is.

The solution is, of course, to put all of the code within the lock()
statement, including retrieval of OldItem. If you put that code within
the if() statement, this has the added benefit of fixing a potential
index-out-of-range error.

However generic I am trying to keep the design, the C parameter will be most
probably representing a key/value pair, exposed to continuous change by a
concurrent "updater" thread. Fine, that's how the application is supposed to
work. When, however, the list of these key/value pair is changed and the
event handler is (safely) fired up, I would want the subscribers to receive
the values (OldItem, "value", etc.) at the moment the list operation was
performed. In other words, I want the updater thread to be locked out while
the event handler is called.

You haven't posted any code that shows how the updater thread would be
locked out. The updater thread can't modify the m_Contents list via the
indexer setter, and I hope it's safe to assume that any other access
to the list is similarly protected.

But what about the objects themselves? Simply locking the list doesn't
prevent access to the items that are referenced by the list. Even if
you lock access to the list in the getter, once you've returned the list
is no longer locked and doesn't protect access to the object that was
returned. And locking the list doesn't necessarily do anything to
prevent access to the individual objects within the list anyway, even by
an event handler called from within the locking of the list or by
threads that event handler might delegate processing of the object to.

It's important to understand that you are dealing with two different
kinds of objects here. The list containing the items, and the items
themselves. Threads needing synchronization will have to using some
form of locking to protect access to those two different kinds of
objects as appropriate, using two different lockable objects.

The only way that locking the list would also lock access to each
individual item in the list is if you use the same "m_Lock" object for
both locking purposes. It would have to be explicit. In addition,
doing so just enhances the chance for deadlock and potentially causes
performance to suffer too. I'm hoping you're not actually doing this.

Now, I understand that the tricky bit is that event handler being invoked on
the subscriber does NOT mean subscriber has successfully consumed those
objects (via custom EventArgs).

Well, first of all...see my comments above. The object being passed to
the event handler isn't the one that's being locked by the setter. So
whether the object has been "consumed" or not doesn't seem relevant.
The only lock you've posted code showing doesn't seem to have anything
to do with the object representing the item the event handler cares about.

Now, you do also pass "this" to the event handler (sender, I assume),
and the event handler could get into trouble as I described trying to
use that. But that's a separate issue, and your comments lead me to
believe that you are more concerned about the use of the C-type object
as opposed to the class containing the m_Objects list.

That in mind...

The only two alternatives I can see to this
problems are:

1) Making a deep clone of the C object, so as to make sure that value is
disconnected from the list and cannot be changed by the updater thread. This
is perhaps the best solution, but introduces the overheads of cloning an
object every single time a notification is fired up. For a simple key/value
pair,
it shouldn't be too bad. Problems may arise when the C object represent a
bigger class.

Out of the two alternatives you've proposed, this is the only one that
makes sense to me. It's not an uncommon solution to some
synchronization issues; by making a true deep copy, you do in fact
completely disconnect the threads that potentially use the same data.

However, whether this solution is actually practical will depend on what
class is represented by C and what it does. Even ignoring the
performance issue you've already mentioned, some classes simply can't be
cloned in any sort of usable way. A common example would be classes
that refer to some sort of unique resource, like a file or network
socket, that sort of thing.

For that reason, as well as the more general performance and design
issues, if you _did_ implement this sort of solution it would make sense
IMHO for the event handler to be responsible for making the clone before
passing it off to some other thread. This avoids having to suffer the
overhead of cloning the object in situations when it's not necessary.
It also clearly delineates what code is responsible for dealing with
that issue (i.e. the event handler, not the indexer).

2) Keeping the list locked until every subscriber has informed the class
that value has been consumed. This means the critical code section would wait
for objects held by every single subscriber, hence opening up a huge
potential for deadlock.

Well, this proposal brings me back to the original question above. That
is, the lock that you've posted does not appear to synchronize access to
the objects within the list. It only seems to synchronize access to the
list itself. If locking using lockObject (or rather, "m_Lock" in the
code you posted) does also lock access to each item in the list, then
that's potentially a serious performance issue.

Beyond that, I would suggest that as far as the indexer setter is
concerned, "consumed" should be a matter of directly observable
behavior. That is, by the time the event handler returns, the object
should be considered "consumed". If the event handler needs the object
to last longer than that or remain locked somehow longer than that, it
should be up to the event handler to deal with that.

Phew. That was a lot, and I'm not really sure I've actually offered
much insight as to what _will_ work.

One thing I'm not entirely clear on is the question of _why_ you want
this sort of locking. The situation as I understand it:

* You've got a class (generic?) that contains a collection of a
type of object C

* This collection is mutable, and can be modified by multiple
threads, thus the need for synchronization of access to the list

* The objects of type C are also mutable. There is an "updater"
thread that can modify them. Presumably other threads access the data
within the objects and this access needs to be synchronized with the
updater thread.

What I don't get is why you care about tying the synchronizing of access
to the list with synchronizing access to the objects within the list.
If you could explain that aspect of your goals a little more clearly,
that might help.

Assuming that goal is really necessary, you will need to incorporate
logic in the setter (and elsewhere) to flag an object of type C as not
updatable. This could be as simple as the object somehow exposing a
locking object (as a property for example) used by the setter and
updater thread, so that the two can coordinate access. Alternatively,
the object could incorporate its own locking API, internally using some
locking mechanism that allows for methods on the object that can be
called, lock, return, then called again to unlock (the Monitor class can
be used for this, for example).

If you can get away from this requirement though, it might make things
simpler for you. That is, the requirement seems to be that you want not
only to know the exact object in the list at the time the list element
is modified, you also want to know the exact state of the object at the
moment that the list element was modified.

My experience is that in a multi-threaded implementation the exact state
of the object is somewhat random. That is, because it all depends on
the exact timing of the code, even with this requirement you never
really know what state the object is going to be in until you look at
it. In many cases it seems to me that you would get equivalent results
(though obviously not identical) by simply ensuring that the state of
the object can be locked long enough to observe it, and let the event
handlers take care of that.

For a given state of the object that could result from the updater
thread modifying the object before the event handler can get to observe
it, the updater thread could have set that state just before you called
the indexer setter. That's what I mean by "equivalent results". So
unless there's some very specific relationship between the updater
thread and the list itself, I don't understand the need to keep the two
synchronized.

So if you can get away from that need, the design becomes a lot simpler.
And simple is good.

Finally, because I haven't quite written enough...

Note that having a design that allows deadlock is not necessarily a bad
thing. It just means you have to be careful to not write code in which
there is actually a potential for deadlock. I mention this in the
context of the question of cloning the objects. It's true that cloning
completely removes the issue of deadlock, when it's practical to
implement that. But as you've already noted, there are particular
drawbacks to cloning as well. If you can avoid writing code that could
deadlock by simply implementing the synchronization correctly rather
than cloning the objects, you get the best of both worlds.

Pete