Vanni wrote:
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
