Locking problem

  • Thread starter Thread starter John Hatpin
  • Start date Start date
J

John Hatpin

I'm pretty new to C#, but I thought I understood the lock() function.
It seems I don't. While debugging, this code:

lock(TList)
{
foreach(UUID tID in TList)
{
Client.Assets.RequestImage(tID, UpdateT) ;
}
}

gives a runtime error with the "in" in "foreach" highlighted, and the
following message: "System.InvalidOperationException: Collection was
modified; enumeration operation may not execute."

Presumably, TList is changing in another thread while that loop is
running - I thought the lock() would prevent that from happening.

Note that RequestImage is a third-party library function, and knows
nothing of the local list TList. UpdateT is a callback event.

I'm using SharpDevelop on WinXP, everything up-to-date.

What's happening here?
 
I'm pretty new to C#, but I thought I understood the lock() function.
It seems I don't. While debugging, this code:

lock(TList)
{
        foreach(UUID tID in TList)
        {
                Client.Assets.RequestImage(tID, UpdateT) ;
        }

}

gives a runtime error with the "in" in "foreach" highlighted, and the
following message: "System.InvalidOperationException: Collection was
modified; enumeration operation may not execute."

Presumably, TList is changing in another thread while that loop is
running - I thought the lock() would prevent that from happening.

Note that RequestImage is a third-party library function, and knows
nothing of the local list TList. UpdateT is a callback event.

I'm using SharpDevelop on WinXP, everything up-to-date.

What's happening here?
Click to expand...

Hello,

Do you also use the lock() in the other thread and everywhere the
TList is modified ?

Eric
 
eric said:
Hello,

Do you also use the lock() in the other thread and everywhere the
TList is modified ?
Click to expand...

Yes, everywhere it's modified. It doesn't need to be locked in places
where it's used read-only, correct?

By the way, this doesn't happen every time, but most times I run that
part of the code. I forgot to mention that.
 
John Hatpin wrote on 9/3/2010 :
Yes, everywhere it's modified. It doesn't need to be locked in places
where it's used read-only, correct?
Click to expand...

Well... If you enumerating it on another thread - even readonly, and
it's modified on another thread, your going to get an exception at the
place where your reading. The list can NOT change during a foreach.

Depending on the usage of the list, then you might want to look at a
more efficient locking mechanism. For example, ReaderWriterLockSlim
(case of multiple readers, single writer)....
 
Peter Duniho formulated on Friday :
Tom said:
[...]
Depending on the usage of the list, then you might want to look at a more
efficient locking mechanism. For example, ReaderWriterLockSlim (case of
multiple readers, single writer)....
Click to expand...

If he has performance problems — that is, he has specific measurements of
performance, has identified the synchronization as the bottleneck, and the
performance does not meet specific performance goals that have been set —
then, yes…use of a many-reader, single-writer lock such as
ReaderWriterLockSlim can improve things.
Click to expand...

Just pointing out alternatives... :)
Otherwise, it's overkill and just complicates the code. If the OP is looking
for something to change (besides fixing his bug, of course :) ), I would add
a dedicated locking object rather than locking on the list instance before I
started messing around with fancy locking mechanisms.

Pete
Click to expand...

Agreed. I almost always lock on a static lock object then actual
instance.

Some of this can be alleviated in .NET 4.0 - with the addition of the
thread safe collections in System.Collections.Concurrent...
 
Peter said:
John said:
[...]
Do you also use the lock() in the other thread and everywhere the
TList is modified ?
Click to expand...

Yes, everywhere it's modified. It doesn't need to be locked in places
where it's used read-only, correct?
Click to expand...

Well, lack of a lock at read-only sites won't affect the loop you're
asking about. But those sites may not work properly either, if they
aren't properly synchronized.
Click to expand...

Yes, I'm getting into the habit of surrounding all updates with a
lock.
That does suggest a synchronization problem, but doesn't prove it 100%.

Again, without a proper code example, it's not possible for any of us to
point to a specific problem. With such an example, it should be easy to.
Click to expand...

I'll post the rest of the relevant code if it's still a problem, but
an odd thing happened. Making sure the list wasn't used elsewhere in
the project, I changed it from public to private - it didn't really
need to be public anyway, the way it is now. And the problem hasn't
happened since.

Thanks to both of you - at least I know the lock is supposed to
function the way I've understood it.
 
Tom said:
John Hatpin wrote on 9/3/2010 :

Well... If you enumerating it on another thread - even readonly, and
it's modified on another thread, your going to get an exception at the
place where your reading. The list can NOT change during a foreach.
Click to expand...

Yes, sorry - I have used lock around foreach. In fact, I added locks
to all access to the list, but it still failed until I made the list
private.
Depending on the usage of the list, then you might want to look at a
more efficient locking mechanism. For example, ReaderWriterLockSlim
(case of multiple readers, single writer)....
Click to expand...

It's only for home use, the simplest of possible cases, but it's
useful to know that more sophisticated locking is possible - thanks.
 
Peter said:
John said:
[...]
I'll post the rest of the relevant code if it's still a problem, but
an odd thing happened. Making sure the list wasn't used elsewhere in
the project, I changed it from public to private - it didn't really
need to be public anyway, the way it is now. And the problem hasn't
happened since.
Click to expand...

Simply changing accessibility, without changing any other code, is not
going to make a threading problem go away. There are two possibilities:

• The modification to the collection was happening via reflection
somewhere, and that code ignores private members.

• Changing the accessibility (or, more likely, some other change you
made at the same time) has somehow just shifted the code around in a
subtle way that makes the timing come out differently, in a way that
reduces the chances of a problem happening during execution.

Note that the latter scenario hasn't actually fixed the bug. It just
makes it less likely to occur.

If you are not 100% certain that the first scenario above is applicable,
then you haven't dealt with the bug yet.
Click to expand...

Indeed, you're right and the bug is still there, ableit much less
frequent. I'll post more details when I have more time.
 
After serious thinking John Hatpin wrote :
Peter said:
John said:
[...]
I'll post the rest of the relevant code if it's still a problem, but
an odd thing happened. Making sure the list wasn't used elsewhere in
the project, I changed it from public to private - it didn't really
need to be public anyway, the way it is now. And the problem hasn't
happened since.
Click to expand...

Simply changing accessibility, without changing any other code, is not
going to make a threading problem go away. There are two possibilities:

• The modification to the collection was happening via reflection
somewhere, and that code ignores private members.

• Changing the accessibility (or, more likely, some other change you
made at the same time) has somehow just shifted the code around in a
subtle way that makes the timing come out differently, in a way that
reduces the chances of a problem happening during execution.

Note that the latter scenario hasn't actually fixed the bug. It just
makes it less likely to occur.

If you are not 100% certain that the first scenario above is applicable,
then you haven't dealt with the bug yet.
Click to expand...

Indeed, you're right and the bug is still there, ableit much less
frequent. I'll post more details when I have more time.
Click to expand...

That would be helpful. You might start by identifying every place that
the variable TList is being accessed. I'm curious, what you do in the
callback function.... What is the purpose of TList? Does each thread
really need to share TList? Or can you simply hand them a private
"snapshot" if you will...

I guess, I'm trying to get you to think if there is a way to reduce
shared state :)

Threading issues can very hard to identify. Even adding debuging code,
such as logging can change the timing so that an issue seems to go away
- when in reality, you've just shifted it (or hidden it until you turn
off or remove the debuging code or it gets run on different hardware :)
)
 
I'm pretty new to C#, but I thought I understood the lock() function.
It seems I don't. While debugging, this code:
Click to expand...
Presumably, TList is changing in another thread while that loop is
running - I thought the lock() would prevent that from happening.
Click to expand...

Avoid multithreading like the plague. Do you like plague? If a dead
animal is rotting by the roadside, do you stop you car, get out, and
eat the road kill with relish? That's multithreading. You are eating
a dead, putrid animal by the roadside that has rabies, and loving it.
Does that describe you?

Even Jon Skeet in his online tutorial recommends avoiding
multithreading.

As a very last resort yes, do use it, but for a lot of stuff where
multhreading is required, like database access or the progress bar or
I/O operations, it's already part of the library (for example both
ADO.NET and Silverlight have asynchronous methods and templates to
use--no need to "lock", just follow the script).

You should not be learning multithreading as a beginner in C#--you
have better things to do. JIMHO.

RL
 
I'm pretty new to C#, but I thought I understood the lock() function.
It seems I don't. While debugging, this code:

lock(TList)
{
foreach(UUID tID in TList)
{
Client.Assets.RequestImage(tID, UpdateT) ;
}
}

gives a runtime error with the "in" in "foreach" highlighted, and the
following message: "System.InvalidOperationException: Collection was
modified; enumeration operation may not execute."
Click to expand...
This suggests that the Client.Assets.RequestImage method may be
altering the list but I've been wrong before.
Presumably, TList is changing in another thread while that loop is
running - I thought the lock() would prevent that from happening.
Click to expand...

The lock method protects the code not the object used as an argument.
MSDN's explanation is far better than any I could give. Plus there is
a light-weight example to help clarify how the lock method is used.

http://msdn.microsoft.com/en-us/library/c5kehkcz(v=VS.100).aspx
Note that RequestImage is a third-party library function, and knows
nothing of the local list TList. UpdateT is a callback event.
Click to expand...
Depending on its purpose and use the callback method could be where
the list is being changed.
I'm using SharpDevelop on WinXP, everything up-to-date.

What's happening here?
Click to expand...

Nothing to worry about, you're learning about locks and locking.

regards
A.G.
 
John said:
I'm pretty new to C#, but I thought I understood the lock() function.
It seems I don't. While debugging, this code:

lock(TList)
{
foreach(UUID tID in TList)
{
Client.Assets.RequestImage(tID, UpdateT) ;
}
}

gives a runtime error with the "in" in "foreach" highlighted, and the
following message: "System.InvalidOperationException: Collection was
modified; enumeration operation may not execute."

Presumably, TList is changing in another thread while that loop is
running - I thought the lock() would prevent that from happening.

Note that RequestImage is a third-party library function, and knows
nothing of the local list TList. UpdateT is a callback event.

I'm using SharpDevelop on WinXP, everything up-to-date.

What's happening here?
Click to expand...

Just to wrap this up, with thanks to everyone for their help:

It turns out that the UpdateT callback was being executed at the same
time as the foreach() loop was running; that callback deleted items
from TList and was thus causing the error. Changing the design to that
removal doesn't happen solved this particular problem.

It was this sort of thing that I was referring to when I used the term
"thread"; I do not do any explicit multithreading, nor do I wish to. I
hadn't realised that callbacks worked in quite that way; I'd imagined
they'd be held in an event queue and processed later.

Again, thanks everyone.
 
Back
Top