Bug 21232 – std.parallelism.parallel reuses thread, leading to stale static data

Status
NEW
Severity
blocker
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2020-09-09T08:24:25Z
Last change time
2024-12-01T16:37:37Z
Keywords
industry
Assigned to
No Owner
Creator
Mathias LANG
Moved to GitHub: phobos#9806 →

Comments

Comment #0 by pro.mathias.lang — 2020-09-09T08:24:25Z
Test code: ``` import std.parallelism; import std.process; import std.range; import std.stdio; import core.atomic; shared int initCount, liveCount; static this () { atomicOp!("+=")(initCount, 1); atomicOp!("+=")(liveCount, 1); } static ~this () { atomicOp!("-=")(liveCount, 1); } class B { int val; static B f() { static B b; if (b is null) b = new B(); return b; } } void runTest (int i){ B b = B.f(); writeln("val is: ", b.val, "; thread id: ", thisThreadID); b.val = 2; } private void main() { foreach (myVal; parallel(iota(0,9))) runTest(myVal); writefln("initCount: %d - liveCount: %d", initCount, liveCount); } ``` Result: ``` val is: 0; thread id: 1149C4DC0 val is: 2; thread id: 1149C4DC0 val is: 2; thread id: 1149C4DC0 val is: 2; thread id: 1149C4DC0 val is: 0; thread id: 70000C529000 val is: 0; thread id: 70000C6B2000 val is: 0; thread id: 70000C62F000 val is: 0; thread id: 70000C5AC000 val is: 0; thread id: 70000C735000 initCount: 6 - liveCount: 6 ``` IMO, this is just a disaster. Since threads are reused, the program will have access to stale, static data. Type safety cannot be guaranteed because the guarantees of module ctor / dtor are just thrown out the window. We found this bug in our custom test runner, which handles priority, and run many threads to speed up testing.
Comment #1 by kinke — 2020-09-09T09:48:02Z
Pardon me, but isn't this to be expected? Creating a thread per iteration would be an absolutely inacceptable overhead in many cases. From the std.parallelism docs: "After creation, a Task may be executed in a new thread, or submitted to a TaskPool for execution. A TaskPool encapsulates a task queue and its worker threads. Its purpose is to efficiently map a large number of Tasks onto a smaller number of threads."
Comment #2 by pro.mathias.lang — 2020-09-10T01:05:06Z
> Pardon me, but isn't this to be expected? Creating a thread per iteration would be an absolutely inacceptable overhead in many cases. It might be unacceptable performance-wise, but I think violating the type-system like this is even less acceptable. `std.parallelism` (and `std.concurrency`) do that on multiple occasions, by not requiring a `shared` delegate and executing things in threads "sometimes". D2 was designed to have a strong distinction between data that is used by multiple threads and data that isn't, with the assumption that the latter is the most common case. `parallel` is just trampling all over this.
Comment #3 by bcarneal11 — 2020-09-10T06:01:24Z
Having library choices for both speed (task pools) and fidelity (heavy weight spawn) seems like a good thing. Anecdotally I've used both and had problems with neither but then I never expected full thread semantics from the task pool.
Comment #4 by robert.schadek — 2024-12-01T16:37:37Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/phobos/issues/9806 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB