Bug 4624 – std.stdio.File and std.typecons.Unique not GC-heap safe

Just took a look at Phobos for struct destructors. Both std.stdio.File and std.typeconst.Unique seem unsafe to store anywhere in the GC heap (in an array or in a class). For std.stdio.File, it's because because the destructor assumes the GC-allocated Impl instance can be dereferenced (which is a risky bet during the collection that the GC will deallocate things in the "right" order): this(string name, in char[] stdioOpenmode = "rb") { p = new Impl(errnoEnforce(.fopen(name, stdioOpenmode), "Cannot open file `"~name ~"' in mode `"~stdioOpenmode.idup~"'"), 1, name); } ~this() { if (!p) return; // @@@BUG@@@ These lines prematurely close the file //printf("Destroying file `%s' with %d refs\n", toStringz(p.name), p.refs); if (p.refs == 1) close; else --p.refs; } In struct std.typecons.Unique(T), unique calls delete on the object it references, but since that object is in the GC heap the same problem arises: it might already have been deallocated: ~this() { writeln("Unique destructor of ", (_p is null)? null: _p); delete _p; _p = null; }

Additionally, I believe std.containers.Array has a race condition when stored in the GC heap. The Array destructor checks the reference count before deciding whether it should free the array's content or not. This reference counter is non-shared, but the collection cycle can run on any thread so there is a race when accessing the reference counter. private struct Data { uint _refCount = 1; size_t _capacity; T[] _payload; this(T[] p) { _capacity = p.length; _payload = p; } } The reference counter should be made shared so it is only accessed using atomic operations. Other fields don't need to be shared because the only time they're accessed in the destructor is when the reference counter falls to zero and the destructor has the only remaining reference. This problem is also present in std.stdio.File (in addition to the other problem above) and std.typecons.RefCounted which both use a non-shared reference counter, rendering them unsuitable for being put in the GC heap.

Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/26b7d3fad37c2cd8e592f4ab4021ba014ff35bd0 Fix std.stdio.File part of Issue 4624

(In reply to comment #2) > Commit pushed to master at https://github.com/D-Programming-Language/phobos > > https://github.com/D-Programming-Language/phobos/commit/26b7d3fad37c2cd8e592f4ab4021ba014ff35bd0 > Fix std.stdio.File part of Issue 4624 This only fixes one side of the std.stdio.File part of this issue: there's still a race between the collecting thread and other threads when accessing and decrementing the reference counter in the destructor (see comment #1 on the issue). The reference counter should only be changed using atomic increment/decrement to prevent that race, at least until the GC can guaranty it will collect objects in the same thread they were created in.

Same occurs with Unique: static assert(isUniformRNG!(Unique!Random)) ... will fail.

(In reply to Joseph Rushton Wakeling from comment #4) > Same occurs with Unique: > > static assert(isUniformRNG!(Unique!Random)) > > ... will fail. Ack. Don't know how this wound up here -- was replying to another bug report. :-(

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/phobos/issues/9889 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB