Issue 2834 - Struct Destructors are not called by the GC, but called on explicit delete.
Summary: Struct Destructors are not called by the GC, but called on explicit delete.
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P3 minor
Assignee: No Owner
URL:
Keywords: spec
: 4442 (view as issue list)
Depends on:
Blocks:
 
Reported: 2009-04-14 08:00 UTC by Rob Jacques
Modified: 2015-02-18 03:38 UTC (History)
15 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Rob Jacques 2009-04-14 08:00:55 UTC
According to the spec: [Struct] "Destructors are called when an object goes out of scope. Their purpose is to free up resources owned by the struct object." And while the heap is always 'in scope', it does delete structs which arguably places them out of scope. Note that an explicit call to delete will run the destructor. At a minimum, the spec should be updated to clearly reflect this limitation. Since memory leaks, open files, etc will probably occur when a struct with a postblit is manually allocated directly on the heap, instead of on the stack or as part of an object, this issue might warrant a compiler warning/error (i.e. an explicit cast is required).
Comment 1 Rob Jacques 2010-07-09 23:20:01 UTC
*** Issue 4442 has been marked as a duplicate of this issue. ***
Comment 2 Rob Jacques 2010-07-09 23:22:39 UTC
From Issue 4442:
Example brought up by Sean Kelly in private correspondence:

struct S1{ ~this() { writeln("dtor"); } }
void main() {
    auto a = S1();
    auto b = new S1();
    delete b;
    auto c  = new S1();
    c = null;
    GC.collect();
}

"dtor" is only printed twice.
Comment 3 nfxjfg 2010-07-10 00:00:11 UTC
Gc finalization is not deterministic. You can't expect it to be called.
Comment 4 Sean Kelly 2010-07-10 08:40:16 UTC
What about structs whose memory are freed by the GC?  Would you expect their dtors to be called?  They aren't.  Try allocating ten million structs in a loop, not one "dtor" line will print.
Comment 5 Sean Kelly 2010-07-10 08:41:54 UTC
Fixing this will probably be fairly involved.  The GC will have to store a TypeInfo reference for each block that needs to be finalized.  The best approach may be to integrate this with precise scanning, since that requires detailed type info too.
Comment 6 bearophile_hugs 2010-07-11 15:16:07 UTC
For the moment the compiler can show a warning when the code allocates on the heap a struct that has a destructor.
Comment 7 bearophile_hugs 2010-07-11 17:24:58 UTC
One case where struct destructors are not called, in this situation it seems simpler for the GC to know what destructors to call:


import core.memory: GC;
import core.stdc.stdio: printf;
struct Foo {
    int x;
    this(int xx) { this.x = xx; }
    ~this() { printf("Foo dtor x: %d\n", x); }
}
void main() {
    Foo[] a;
    a.length = 2;
    a[0].x = 1;
    a[1].x = 2;
    // delete a;
}


(I am not sure, but a type information can be useful in arrays, maybe to fix bug 2095 too.)
Comment 8 Max Samukha 2010-11-18 03:39:17 UTC
So what is the verdict? Should we simply specify that struct destructors are not automatically called except in RAII and remove the struct-in-class special case?

BTW, there are other problems (serious IMO):

auto ss = new S[10];
ss.length = 5;
delete ss; 

Destructors are not called on the last 5 elements.

----
auto ss = new S[10];
ss ~= ss;
delete ss;

We have a nasty problem when destructors are called on the appended elements because postblits was not run for them during append.

etc

Essentially, operations on arrays of structs with postblits/dtors defined are currently unusable.
Comment 9 Max Samukha 2010-11-18 03:59:12 UTC
(In reply to comment #8)
> We have a nasty problem when destructors are called on the appended elements
> because postblits was not run for them during append.

I meant the problem is the destructors called on objects that have not been postblitted.
Comment 10 Dicebot 2014-02-28 04:53:17 UTC
Updated flags as it affects all platforms and is general language defect.
Comment 11 rswhite4 2014-02-28 05:35:48 UTC
Maybe we should put a bounty on it.
Comment 12 Benoit Rostykus 2014-07-03 20:59:59 UTC
AdRoll (the company I work for) just put a $1000 bounty on this bug:
https://www.bountysource.com/issues/2900969-struct-destructors-are-not-called-by-the-gc-but-called-on-explicit-delete

We are really excited about D, and would generally love to see any work on improving D's GC!
Comment 13 Jonathan M Davis 2014-07-04 01:03:02 UTC
I'd strongly argue that we make it so that struct destructors get called when the memory for a struct is freed on the GC heap (though that pretty requires what precise heap scanning requires as Sean points out). However, it should be pointed out that there's no guarantee (and likely never will be a guarantee) that everything on the heap will be collected, which would mean that it will never be guaranteed that all struct destructors will be called when on the heap - just that they would be called in the cases where the struct is collected and freed by the GC.

So, while we should be able to make it so that struct destructors on the GC heap get called much more (as opposed to never), it will almost certainly be the case that the non-GC heap (along with some kind of manual memory management or reference-counting) will have to be used if it's _required_ that the destructor be run (though technically, not even that would guarantee it, since the program could still segfault, or an Error could be thrown, or the computer could lose power or some other incident could occur which would make it so that the program terminated prematurely, in which case, the destructors wouldn't be run).
Comment 14 Jonathan M Davis 2014-07-04 01:04:03 UTC
(In reply to Benoit Rostykus from comment #12)
> AdRoll (the company I work for) just put a $1000 bounty on this bug:

That's quite the bounty.
Comment 15 Orvid King 2014-07-04 17:53:29 UTC
Would making all the writeln's in http://dpaste.dzfl.pl/fbb4a15cda14 print out be an acceptable solution to this issue? (that is, making heap-allocated structs call their destructors)
Comment 16 David Nadlinger 2014-07-04 18:27:35 UTC
(In reply to Orvid King from comment #15)
> Would making all the writeln's in http://dpaste.dzfl.pl/fbb4a15cda14 print
> out be an acceptable solution to this issue? (that is, making heap-allocated
> structs call their destructors)

As far as I can see, this would not necessarily test structs contained in other structs/classes/arrays.
Comment 17 Orvid King 2014-07-04 22:21:53 UTC
https://github.com/D-Programming-Language/druntime/pull/864

It's not feasibly possible to call destructors on heap-allocated arrays of structures, however this PR does implement the calling of destructors for structs allocated directly on the heap.
Comment 18 Jonathan M Davis 2014-07-04 22:40:59 UTC
(In reply to Orvid King from comment #17)
> https://github.com/D-Programming-Language/druntime/pull/864
> 
> It's not feasibly possible to call destructors on heap-allocated arrays of
> structures, however this PR does implement the calling of destructors for
> structs allocated directly on the heap.

As I understand it, it should be possible to call the destructors for structs in arrays if we have the additional type information required for precise heap scanning, but until we add that, we're stuck. But if you can make it so that the destructors get called in other circumstances, that's a great improvement regardless.
Comment 19 Orvid King 2014-07-05 03:43:57 UTC
We still wouldn't be able to call the destructors in structs for arrays even 
with precise heap scanning, because we can't know that each value in the array 
is actually valid. Take for instance an array of File's, it's not valid to call 
the destructor of File.init, so we could compare the value of an element of an 
array of structs to it's init value, but who's to say that element was ever 
initialized in the first place? How would we detect that?

I think the best thing to do with the arrays of structs that have destructors 
is to simply make them illegal, and to present as an alternative, an array of a 
structure that you have defined and has an inner alias this to the original 
structure, but rather than having a destructor, has a named method that the 
user would have to call themselves for each element that they know is 
initialized.
Comment 20 Dmitry Olshansky 2014-07-05 09:14:37 UTC
(In reply to Orvid King from comment #19)
> We still wouldn't be able to call the destructors in structs for arrays even 
> with precise heap scanning, because we can't know that each value in the
> array 
> is actually valid. Take for instance an array of File's, it's not valid to
> call 
> the destructor of File.init, so we could compare the value of an element of
> an 
> array of structs to it's init value, but who's to say that element was ever 
> initialized in the first place? How would we detect that?

It's perfectly valid to call destructor on T.init. In fact compiler will always do so with stack-allocated instances.
Comment 21 Rainer Schuetze 2014-07-05 10:32:54 UTC
(In reply to Orvid King from comment #19)
> We still wouldn't be able to call the destructors in structs for arrays even 
> with precise heap scanning
[...]
> How would we detect that?

The array handling in lifetime.d should supply a type info with a destructor that iterates over the elements and calls their destructor. You might have to generate that typeinfo at runtime, though.
Comment 22 Orvid King 2014-07-18 19:38:29 UTC
Ok, so, to give an update on this issue, I've now implemented the invoking of destructors on heap allocated structs, as well as arrays of structs. It required a fix to a bug in DMD with the delete operator, as well as a change (which I still need to make), to the test suite so that it doesn't allocate in a destructor.

The DMD PR is here:
https://github.com/D-Programming-Language/dmd/pull/3727
And the druntime PR, as mentioned farther up, is here:
https://github.com/D-Programming-Language/druntime/pull/864
Comment 23 Orvid King 2014-08-27 15:02:39 UTC
And another update.

The fix to the other issue that this encountered has already been merged, and the main DRuntime PR has been running green on the autotester for a while now. It's just awaiting a final review and merge. I've also been doing my local projects with the man DRuntime PR included, and have yet to encounter any issues on win32, so I believe that it is stable.
Comment 24 Andrei Alexandrescu 2014-08-28 05:42:30 UTC
Ready to take the big step with 2.067?
Comment 25 Orvid King 2014-08-28 17:05:25 UTC
Fine by me.
Comment 26 hsteoh 2014-08-29 02:33:46 UTC
Let's do it. This issue has been waiting since 2009. It's about time we did this right.
Comment 27 Andrei Alexandrescu 2014-08-29 21:18:27 UTC
Could you please add a function to druntime callStructDtorsDuringGC(bool)? That would give a chance people who have issues with the called destructors to revert back to the old behavior until they fix their code.
Comment 28 Orvid King 2014-08-30 16:46:45 UTC
Where should I add it? It can't be in rt.lifetime, because that's not exposed 
to the user.
Comment 29 Andrei Alexandrescu 2014-08-30 20:49:40 UTC
core.memory?
Comment 30 github-bugzilla 2015-01-15 10:49:12 UTC
Commit pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/dbbea7c5bd4d8936dc8cb523b65f771842f2a487
Merge pull request #864 from Orvid/structDestructors

Fix Issue 2834 - The GC will now call destructors on heap allocated structs
Comment 31 github-bugzilla 2015-02-18 03:38:45 UTC
Commit pushed to 2.067 at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/dbbea7c5bd4d8936dc8cb523b65f771842f2a487
Merge pull request #864 from Orvid/structDestructors