ThreadSafeLogThreadSafetyTest sometimes fails

[ascheibe]

It seems there is a Deadlock in the ThreadSafeLog which occurs sometimes since we upgraded to VS2012 Unit Tests.

[abeham]

We could aim to go with a simpler, but less performant solution that doesn't allow concurrent reads. If we replace the ReaderWriterLockSlim with a lock hopefully the deadlock would vanish. I don't think this is a performance critical part of the application.

I have looked at this code twice already and I couldn't find a problem nor reproduce the error.

[gkronber]

The problem could be in LogView. In ThreadSafeLog access to the shared variable messages seems to be handeled correctly. However, the event handlers for message added and cleared are again unsynchronized.

[gkronber]

The loop with messages.RemoveAt(0) in Log.cs CapMessages() is also bad style.

I think it would be possible to replace the messages list with a thread-safe queue to simplify the thread-safe log.

[ascheibe]

Replying to gkronber:

The loop with messages.RemoveAt(0) in Log.cs CapMessages() is also bad style.

I think it would be possible to replace the messages list with a thread-safe queue to simplify the thread-safe log.

I think this would be a good solution, though we need to find a way to work around the problem that the base class Log contains a storable list of strings (messages). ~On the other hand i think nobody ever saved a log so i think we can break persistence?~ (just saw that engine uses Log) Or write some backwards compatibility code.

[abeham]

Replying to gkronber:

The problem could be in LogView. In ThreadSafeLog access to the shared variable messages seems to be handeled correctly. However, the event handlers for message added and cleared are again unsynchronized.

LogView is not present during the execution of the unit test.

[gkronber]

Replying to abeham:

Replying to gkronber:

The problem could be in LogView. In ThreadSafeLog access to the shared variable messages seems to be handeled correctly. However, the event handlers for message added and cleared are again unsynchronized.

LogView is not present during the execution of the unit test.

Hm, that's right. Any other handlers registered to the log?

[abeham]

change to the threadsafelog

[abeham]

I attached a patch where ThreadSafeLog uses a ConcurrentQueue<string> and doesn't derive from Log.

[gkronber]

Patch looks good to me.

[ascheibe]

r11596 applied abeham's ThreadSafeLog patch

[ascheibe]

r11597 added a unit test for the cap method of the ThreadSafeLog

[ascheibe]

I also think the patch is fine, the only question that I have is: Do we really need the capLock lock object? I know that the checking in the CapMessages() method and the removal of a message is not an atomic operation, so there could be a problem. I have written a unit test for it and if i remove the locking, it still works. So i don't know if we should just remove it or leave it as it is?

[ascheibe]

r11600 fixed name of new unit test

[abeham]

We didn't experience any ThreadSafeLog fails so far. I hope that the problem is fixed. The lock in CapMessages needs to be in place, otherwise you would be dequeueing too many messages.

Please release r11596, r11597, r11600

[ascheibe]

r11821 merged r11596,r11597,r11600 into stable branch