Improved semantics for destroy()

[andralex]

This supersedes https://github.com/dlang/druntime/pull/2124 and previous attempts. Apologies for the override but it seemed easier to simply write this than discuss what to do over several sessions. cc @JinShil

[dlang-bot]

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[JinShil]

This is better than my prior understanding of @andralex's specification as, at least, reference types and value types are treated uniformly. However, I still don't understand why having a destructor or not having a destructor should result in different behavior.

And, it looks like we'll have to do something about Higgs. cc @sbstp See the Jenkins "details".

[sbstp]

I'm not the author of Higgs, you should probably cc @maximecb.

[maximecb]

Oha @JinShil.

So, it's been a long time since I've looked at this code, but this might be the source of your problem: https://github.com/higgsjs/Higgs/blob/master/source/runtime/gc.d#L914

[12345swordy]

no attempts of fixing bugs regarding the class deconstructor when it comes to attributes?

[JinShil]

Breaking Higgs

I removed one destructor from Higgs in an attempt to make it compatible with this PR, but there are still 3 more that need attention:

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/object.d#L512 https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/vm.d#L805 https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/gc.d#L87

Alternate Proposal

I propose decoupling destruction from re-initialization for all cases, and delegating the decision and method of re-initialization to the user. That puts the burden on the user to understand what they need to do and why, but I believe that's responsibility the user opts into when they choose to implement a destructor. Consider the following contrived examples.

Method 1: Use a boolean flag to indicate whether or not the object has already been destroyed

struct S 
{
    void* p;
    bool destroyed = false;
    this(size_t size)
    {
        p = allocateResource(size);  // may not have the same semantics as `malloc`
    }
    ~this() 
    {
        if (!destroyed)
        {
            deallocateResource(p);   // may not have the same semantics as `free`
            destroyed = true;
        }
    }
}

Method 2: Use a special state to indicate whether or not the object has already been destroyed

struct S 
{
    void* p;
    this(size_t size)
    {
        p = allocateResource(size);   // may not have the same semantics as `malloc`
    }
    ~this() 
    {
        if (p == null)
        {
            deallocateResource(p);   // may not have the same semantics as `free`
            p = null;
        }
    }
}

Method 3: Re-destuction is benign due to the semantics of the resource deallocation

struct S 
{
    void* p;
    this(size_t size)
    {
        p = allocateResource(size);  // same semantics as `malloc`
    }
    ~this() 
    {
        deallocateResource(p);     // same semantics as `free`
        p = null;
    }
}

Learning from C#

There may also be some wisdom to be found in C#'s dispose pattern documented here: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern. It is Microsoft's solution to basically the same problem.

[JinShil]

no attempts of fixing bugs regarding the class deconstructor when it comes to attributes?

That's out of scope for now, and probably needs changes to be made in DMD.

[andralex]

@JinShil the destructors in Higgs look reasonable. In what way are they broken?

As a general disposition, we're going toward more, not less, safety. So the situation in which we make destroy leave the object in an unsafe state for destruction is not acceptable. Yes, per your examples there are numerous ways in which user code can make the destructor safe. (Those include of course overwriting the object with .init, which is exactly what destroy does!) But it is impossible for the compiler to assess that the changes make the destroy/~this combo safe.

[JinShil]

In what way are they broken?

I believe it is their mere existence, causing the object to be re-initialized, that causes problems for the Higg's test suite. But, I need to look into it in more detail to be sure.

[JinShil]

@andralex How confident are you in this implementation? For some reason, noone from team DRuntime seems very interested in it (or maybe you're just too scary). Are you willing to play the dictator with this, or are you looking for feedback?

[andralex]

@JinShil thanks for asking! :) I think the intent is what we need; of course it can be improved, and also the implementation may have issues. So feedback is welcome.

Generally destroy needs to rely on what we already have in the language instead of requiring new paraphernalia or asking the user to rewrite their destructors in a certain way.

The mechanisms at our disposal are the following:

• Destructors get rid of an object's extra resources.
• Destructors leave the object in an invalid state; adding a requirement here is not tenable.
• Every object (even those with @disable this();) has a well-defined init state
• The init state does not allocate any resources and can be copied with memcpy
• At least for objects without @disable this();, an object in its initial state must be destroyable, otherwise trivial code such as { T t; } would be incorrect.
• We can use introspection to figure out whether an object has a destructor or not.

The fundamental approach of this PR is simple - calling destroy should call the destructor, if any, and leave the object in a valid destroyable state. So if the object has no destructor there's no need to do anything at all - leave the object as is, it's as destroyable as it was before calling destroy. If the object does have a destructor we should call it and then repaint the object with its initial state - the one that we can reasonably assume it's safely destroyable. If we remove from this behavior we allow for unsafe code (people call destroy and then the destructor gets automatically invoked). If we add to this behavior we do work for naught (as was the case before - why should destroy on an int set it to zero? Every int has as much resources (none) and as many rights as others). If we add requirements to the destructor we break all existing destructors. If we add compiler support we incur overhead. There's really not a lot of wiggle room here.

I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves?

[wilzbach]

I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves?

See Mike's comment above. They can be seen on the Project Tester:

https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fdruntime/detail/PR-2126/2/pipeline

For local reproduction, do sth. like:

git clone https://github.com/higgsjs/Higgs
cd Higgs
dub --compiler=/home/seb/dlang/dmd/generated/linux/release/64/dmd

[JinShil]

I'm unclear on how Higgs got broken, and probably we need to get to the bottom of that. Where can be failures be seen and how do they manifest themselves?

The symptom appears in the Higgs test suite where the unitttests are specifically testing if an object is in a certain state:

Assertion failed (runtime/gc.d@472): gcForward: object not in from-space heap

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/gc.d#L472

I believe this is caused simply by the existence of some of destructors in the Higgs code base. I already removed 1 empty constructor which solved one error, but another appeared. There are current 2 more empty constructors in Higgs:

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/object.d#L512 https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/vm.d#L805

I tried to remove those as well, but it didn't solve the problem we're currently experiencing.

The current error appears to be due to the following destructor, which is not empty:

https://github.com/higgsjs/Higgs/blob/dbe1fb993addfef98e900df6fe2e1e3afe8ecae4/source/runtime/gc.d#L87

I tried to fix that also to make it compatible with this PR, but ultimately failed. I don't understand the code or the test suite logic well enough to make the change.

If you are convinced that this PR is the way to go, and are willing to exercise your authority, given the lack of reviews, I propose the following procedure:

1. Remove Higgs from the Jenkins test suite
2. File an issue in the Higgs repository and leave it to the authors there to fix it
3. Merge this PR
4. If and when Higgs is fixed to make it compatible with this PR, we can re-add it to the Jenkins test suite.

Note that I am not a member of team DRuntime, so if you don't want to merge your own PR, you'll still have to convince someone to review this PR. I'm not crazy about these semantics, but I also don't have any other solution. I'd be hesitant putting my name next to the merge notice even if I were on team DRuntime, but I'd do it if enough time passed without anyone else stepping up with an alternate solution.

Pinging @klickverbot @schveiguy @MartinNowak @rainers @ZombineDev @DmitryOlshansky @maximecb for help.

[12345swordy]

!ping

[ntrel]

Optimization: If a class/struct aggregate does not define a destructor itself, but one of its fields defines a destructor, we need to do field = field.init so the field destructor can be called again later. The aggregate's generated destructor will not access any fields that don't have a destructor, so we do not need to reinit those. This pull seems to always reinit all fields of the aggregate (which is fine, but overkill).

Also, I like the new semantics of destroy, but I expect people have relied on it always doing the reinit. I read the discussion in #2115, and even if the doc description was vague (until recently?), the [documented] unittests were not - they clearly show reinit, implying that users can rely on it. I suggest this pull either uses a different function name or somehow uses deprecation to make users aware of the change.

[andralex]

@ntrel we won't cater to people relying on undocumented behavior. This not only creates difficulties for this particular PR, but creates a dangerous precedent whereby we dig ourselves in bureaucracy.

[ntrel]

people relying on undocumented behavior

@andralex I suggest we revert #2054 ASAP unless this is merged now. #2054 was merged on 27 Jan (it also says you approved those changes).

Observation: The hasElaborateDestroy changes and the destroy(T[]) changes are orthogonal and could have been separated.