C# lock leads to freeze

Question

i have this list of sounds:

     List<SourceVoice> runningInstances;

i attach an event to a sound object so that i remove it from the list when it is stopped.

    sourceVoice.StreamEnd += delegate
            {
                lock (runningInstances)
                {
                    runningInstances.Remove(sourceVoice);
                }
            };

and i also have this stop function, which is called from any thread.

    public void stop(int fadeoutTime)
    {
        lock (runningInstances)
        {
            foreach (var sourceVoice in runningInstances)
            {
                if (!sourceVoice.IsDisposed)
                {
                    sourceVoice.Stop();
                    sourceVoice.FlushSourceBuffers();
                    sourceVoice.DestroyVoice();
                    sourceVoice.Dispose();
                }
            }
            runningInstances.Clear();
        }
    }

i thought that since i make the event a delegate, it will always wait until the object is unlocked. however it seems that it freezes there.

*"i thought that since i make the event a delegate"* - can you please explain that thought in more detail? Because whatever you thought there is completely wrong, but without the exact thought process it is hard to explain what exactly is wrong with it and why. — Daniel Hilgarth, Jul 09 '13 at 13:01
i thought delegates pick a thread from the threadpool and run it there. — clamp, Jul 09 '13 at 13:03
Nope, they don't. There isn't even anything similar to that. It's just not what is happening. The delegate is executed on the thread that is chosen by the class that raises the event. — Daniel Hilgarth, Jul 09 '13 at 13:05
then what is the difference between `event += delegate {};` and `event += () => {};` ? — clamp, Jul 09 '13 at 13:05
The difference is only in the syntax. There is no functional difference. — Daniel Hilgarth, Jul 09 '13 at 13:06
@DanielHilgarth thx, if you put that as an answer i can accept it — clamp, Jul 09 '13 at 13:12
does your sourceVoice.Stop(); raise the sourceVoice.StreamEnd event? — Rezoan, Jul 09 '13 at 13:13
if stop(int fadeoutTime) method and sourceVoice.StreamEnd event are fired at the same time it will gonna stuck. it will be more clear if you can say when the sourceVoice.StreamEnd event fires — Rezoan, Jul 09 '13 at 13:19
It seems there is another issue too. You enumerate runningInstances and the event handler changes the collection in the same time. Just copy all items into another collection e.g. ToArray() method and stop items without locking. — Alexey, Jul 09 '13 at 13:25
@clamp: You are welcome. But while you were really wrong with your interpretation of `delegate`, I still don't know why your code freezes and as such I actually have no information I could post as an answer... — Daniel Hilgarth, Jul 09 '13 at 13:27
well, i have now changed it to a delegate::BeginAsync call and it doesnt freeze anymore. it probably did because, as suspected by Rezoan, StreamEnd was called from within Stop. — clamp, Jul 09 '13 at 13:39
@clamp Even if it was, that wouldn't be the problem, unless there is a problem in code you haven't shown us. — Servy, Jul 09 '13 at 13:40
@Servy: The only plausible explanation is that `Stop` spins up another thread and waits for it to finish (for whatever reason) with the thread raising `StreamEnd`. A classical dead-lock. — Daniel Hilgarth, Jul 09 '13 at 13:45
sry, i meant delegate.BeginInvoke() with a delegate.EndInvoke() inside its AsyncCallback — clamp, Jul 09 '13 at 13:56
@Sevy, hmmm, you are right. a double lock shouldnt freeze. but now i cant think of any other relevant parts in my code... — clamp, Jul 09 '13 at 13:58

H H · Answer 1 · 2013-07-09T17:03:17.470

There are 2 possibilities:

the event is raised on the same thread as sourceVoice.Stop();. The lock() {} has no function because it is re-entrant but it is also harmless. The Items should already have been removed when Clear() is called.
the event is raised on another (threadpool) thread. This is up to sourceVoice.Stop(). The lock() will block the event handling until after runningInstances.Clear(). After that the handlers will run and removing from an epty List<> is not an error.

Neither would cause any 'freezing', so there must be something relevant in code we don't see.

jlew · Answer 2 · 2013-07-09T13:13:49.367

-1

Delegates are just callbacks, they don't make any guarantees about threading. You may want to check out the ConcurrentBag class, which is already thread-safe, so you can avoid worrying as much about the locking with respect to the collection.

edited Jul 09 '13 at 13:13

answered Jul 09 '13 at 13:08

jlew

10,491
1
35
58

score -2 · Answer 3 · answered Jul 09 '13 at 13:10

-2

It looks like one of the calls within the lock scope of the stop method is probably causing the StreamEnd event to fire. You could test for this by stepping through the code in the stop method a seeing if it jumps into the event. I would hazard a guess that its the sourceVoice.Stop() call.

answered Jul 09 '13 at 13:10

Peter Holmes

622
4
5

1

Even if this were the case, it wouldn't result in a deadlock given the code we can see. If the event is in the same thread then the lock, being reentrant, isn't a problem. If it's another thread, then it will be able to wait as the original thread isn't also waiting on it. – Servy Jul 09 '13 at 13:40
@Servy you are correct, however the SourceVoice class is partially native code, so I wasn't sure if that would have an effect on detecting re-entrancy. I also suspect that Henk is also correct. I have implemented the code in question and I don't get any deadlocks, so I suspect the fault is caused by code that the OP has not provided. – Peter Holmes Jul 09 '13 at 18:04

score -2 · Answer 4 · edited May 23 '17 at 12:05

-2

You can change your stop method as below if sourceVoice.Stop() always raise the sourceVoice.StreamEnd event.

public void stop(int fadeoutTime)
    {
            foreach (var sourceVoice in runningInstances.ToList<SourceVoice>())
            {
                if (!sourceVoice.IsDisposed)
                {
                    sourceVoice.Stop();
                    sourceVoice.FlushSourceBuffers();
                    sourceVoice.DestroyVoice();
                    sourceVoice.Dispose();
                }
            }

    }

To know about .ToList() you can see

ToList()-- Does it Create a New List?

edited May 23 '17 at 12:05

Community

1
1

answered Jul 09 '13 at 13:41

Rezoan

1,745
22
51

If he did this then it wouldn't actually do what he wants; he'd be constantly modifying copies of the list and he doesn't want to do that. What's the point of doing something like `runningInstances.ToList().Clear();`? Why create a copy just to clear it? Also note the generic argument to `ToList` will be inferred, there's no need to write it out. – Servy Jul 09 '13 at 13:50
i know runningInstances.ToList().Clear(); is not necessary. when sourceVoice.StreamEnd is fired on call of sourceVoice.Stop(); but OP is unable to tell me that if this happen. see the comment. that's why i keep the same source as he mensioned. But why this minus vote i don't understand. the solution i give it should remove the freezing. cause i think stop will call when there is any need to stop all the source voice. and it will make seance @Servy – Rezoan Jul 09 '13 at 14:11
If you know it's not necessary, why would you put it in there? Throwing in the `ToList` calls makes the entire code do nothing. It's like telling the OP if he just removed all of the code in entirely it would resolve the deadlock. Yes, perhaps it would, but then he still wouldn't be solving his actual problem, so it wouldn't matter. – Servy Jul 09 '13 at 14:12
cause OP is unable to tell me that if sourceVoice.Stop() raises the sourceVoice.StreamEnd event. if so there is no need of runningInstances.ToList().Clear(); and reading the question as i understand he is facing some problem about deadlock/freezing. and i am trying to give him a solution @Servy – Rezoan Jul 09 '13 at 14:18
If you can't answer the question until the OP answers a clarifying question the *don't answer the question*. Posting an answer when you don't think that you have enough information to answer it is a terrible idea. In any case, regardless of the answer to your question, creating a new temp list and clearing it does nothing but waste some CPU time; it never does anything, but the fact that you're no longer clearing the real list means it won't actually work as it should have, regardless of the answer to your question, so this is wrong no matter what. – Servy Jul 09 '13 at 14:20
i have now edited the answer and i think it is makes sense now. @Servy – Rezoan Jul 09 '13 at 14:30
Now you're just not doing something that you need to do, as I've told you several times. The OP wants to clear the list at that time, you're not, therefore you're not meeting the requirements of the program. (And also probably not stopping the deadlock, as it was deadlocking before it got there anyway.) – Servy Jul 09 '13 at 14:31
if sourceVoice.Stop() raises the event sourceVoice.StreamEnd it should clear the runningInstances isn't it @Servy – Rezoan Jul 09 '13 at 14:34
Your comment doesn't make any sense. The problem isn't that he's getting a concurrent modification exception, so iterating over a copy of the list won't stop a deadlock, just that one exception (that he's not saying he's getting). And removing the `Clear` makes the program not work, but wouldn't fix a deadlock. – Servy Jul 09 '13 at 14:39
concurrent modification exception only occurs when you are trying to modify a single instance of object form multiple thread. .ToList() makes a new instance. @Servy – Rezoan Jul 09 '13 at 14:42
Yes, and my entire point is that that isn't the problem the OP is saying he has; he's saying he has a deadlock. You're not fixing that deadlock. – Servy Jul 09 '13 at 14:45
so you are saying my solution will also create deadlock. if so could you please point out how? – Rezoan Jul 09 '13 at 14:47
I'm saying that if the OP's code causes a deadlock then this isn't doing anything that would fix it. If I knew exactly why the OP's code was deadlocking I'd post an answer. Since the OP is claiming that his code is deadlocking, care to point out why his code is deadlocking and how your change addresses that issue? – Servy Jul 09 '13 at 14:52
@Servy, He is using a new List(.toList()) ie. not the same list, so there is no reason to lock the same list twice and no deadlock – Mehbube Arman Jul 10 '13 at 05:27
Hey Servy let me give you an example: Suppose you are holding a glass of water in your hand and you are determine to not release the glass until the full water is finished/glass is empty. but someone at the same time trying to grab the glass and deduct some of the water form it. that is not gonna happen. so what exactly is happening there is a stuck situation. that's the problem in the OPs code. – Rezoan Jul 10 '13 at 05:40
@Rezoan Neither section of code is trying to add items to the collection. One is iterating it, one is removing from it. The premise that items are being added more quickly that the collection can be iterated can't be happening. And in any case, the OP would be getting concurrent modification exceptions if he were iterating while the collection was modified; that he isn't means that's not what's going on. – Servy Jul 10 '13 at 13:48
@MehbubeArman But the code also doesn't *work* as it's not doing what the OP intended the entire snippet to do to begin with. As I can't see why the OP's code is deadlocking to begin with I can't say if this change has fixed it; if it does fix it it most certainly doesn't explain how, making it a poor answer, and since it no longer meets the requirements whether or not it deadlocks is irrelevant. As I said before, I could just delete the whole snippet and be sure it won't deadlock, it just won't work as intended either. – Servy Jul 10 '13 at 13:50

C# lock leads to freeze

4 Answers4