Bug 19956 – Subclassing Thread with synchronized (this) may deadlock

Status
NEW
Severity
normal
Priority
P3
Component
druntime
Product
D
Version
D2
Platform
x86_64
OS
Linux
Creation time
2019-06-13T10:23:58Z
Last change time
2024-12-07T13:39:21Z
Assigned to
No Owner
Creator
FeepingCreature
Moved to GitHub: dmd#17179 →

Comments

Comment #0 by default_357-line — 2019-06-13T10:23:58Z
Consider the following program: import core.thread; import core.time; class BadThread : Thread { this() { super(&run); } void run() { synchronized (this) { Thread.sleep(1.seconds); (new Thread({})).start; } } } void main() { (new BadThread).start; Thread.sleep(500.msecs); } This program should terminate after a second. However, it deadlocks. What happens here? The sequence of events is such: Thread 1: starts Thread 2 Thread 1: sleeps Thread 2: locks BadThread Thread 2: sleeps Thread 1: wakes up Thread 1: exits main, runs thread_joinAll Thread 1: thread_joinAll: locks Thread.slock Thread 1: thread_joinAll: checks BadThread.isDaemon Thread 1: BadThread.isDaemon: tries to grab the monitor of BadThread, which is locked in Thread 2 Thread 1 blocks. Thread 2: wakes up Thread 2: tries to start a new Thread Thread 2: Thread.start: tries to grab slock Thread 2 blocks. And the program is deadlocked.
Comment #1 by dfj1esp02 — 2019-06-14T07:51:48Z
Nice; one reason why synchronized methods are now considered an antipattern in .net is because object's monitor is public and there's no way to restrict access to it, so it can accidentally end up being used in unrelated scenarios.
Comment #2 by default_357-line — 2019-06-26T11:18:50Z
Sure, but even being protected would not have helped here. Maybe `Thread` should be `final`?
Comment #3 by dfj1esp02 — 2019-06-27T08:42:44Z
Try to lock a dedicated mutex in BadThread instead of this and see if there's a deadlock.
Comment #4 by dfj1esp02 — 2019-06-27T08:45:58Z
Recommended lock pattern in .net: import core.thread; import core.time; class BadThread : Thread { Object locker; this() { locker=new Object; super(&run); } void run() { synchronized (locker) { Thread.sleep(1.seconds); (new Thread({})).start; } } }
Comment #5 by robert.schadek — 2024-12-07T13:39:21Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/17179 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB