Opened 4 years ago

Closed 3 years ago

#2112 closed defect (done)

ThreadSafeLogThreadSafetyTest sometimes fails

Reported by: ascheibe Owned by: ascheibe
Priority: medium Milestone: HeuristicLab 3.3.11
Component: Tests Version: 3.3.8
Keywords: Cc:

Description

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

Attachments (1)

threadsafelog.patch (7.4 KB) - added by abeham 4 years ago.
change to the threadsafelog

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by 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.

comment:2 follow-up: Changed 4 years ago by 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.

comment:3 follow-up: Changed 4 years ago by 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.

comment:4 in reply to: ↑ 3 Changed 4 years ago by 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.

Last edited 4 years ago by ascheibe (previous) (diff)

comment:5 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 4 years ago by 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?

Changed 4 years ago by abeham

change to the threadsafelog

comment:7 Changed 4 years ago by abeham

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

comment:8 Changed 4 years ago by gkronber

Patch looks good to me.

comment:9 Changed 3 years ago by abeham

  • Status changed from new to assigned

comment:10 Changed 3 years ago by gkronber

  • Milestone changed from HeuristicLab 3.3.x Backlog to HeuristicLab 3.3.11

comment:11 Changed 3 years ago by ascheibe

r11596 applied abeham's ThreadSafeLog patch

comment:12 Changed 3 years ago by ascheibe

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

comment:13 Changed 3 years ago by 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?

comment:14 Changed 3 years ago by ascheibe

  • Owner changed from ascheibe to abeham
  • Status changed from assigned to reviewing

comment:15 Changed 3 years ago by ascheibe

r11600 fixed name of new unit test

comment:16 Changed 3 years ago by abeham

  • Owner changed from abeham to ascheibe
  • Status changed from reviewing to readytorelease

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

comment:17 Changed 3 years ago by ascheibe

  • Resolution set to done
  • Status changed from readytorelease to closed

r11821 merged r11596,r11597,r11600 into stable branch

Note: See TracTickets for help on using tickets.