Should destroyObject also replace .destroy()?

[jjspace]

While reviewing #12129 I finally noticed the if !isDestoryed() > .destroy() pattern and it felt very odd to me.

if (!this._environmentMapManager.isDestroyed()) {
  this._environmentMapManager.destroy();
}

My comment:

It feels like the responsibility for this check should live in the destroy function itself. I feel like you should be able to just call destroy() regardless and let it handle checking if it's already destroyed and essentially turn into a noop function. Then in this function the logic would just be "We know we want to guarantee this is destroyed so call destroy()" and no wrapper check for if it's already destroyed?

Looking at the destroyObject function I see we delete all the properties and replace the .isDestroyed function with a function that only returns true.

cesium/packages/engine/Source/Core/destroyObject.js

Line 53 in 192ab06

object.isDestroyed = returnTrue;

I think a potentially easy path forward to streamline this pattern and fit the way I expected it to work would be to also (or maybe instead of) replace the .destroy() function with a noop () => {}.

This would allow us to rewrite that original example as just one line and not have to care about checking first. The responsibility is with the original object even though it's all been deleted by destoryObject

this._environmentMapManager.destroy();

[javagl]

I just stumbled over this issue. And I agree that some of this could be reviewed, in terms of convenience or the potential for error. Some of the complexity here cannot be avoided: The lifecycle management of GL-objects has to be explicit. And some ideas behind the destroy/isDestroyed/destroyObject functions certainly make sense. However, I don't know all the details about how they evolved, so everything that I say here is to be taken with a grain of salt.

In general, I agree that it would be nice if the pattern of
if (!this.thing.isDestroyed()) this.thing.detroy();
could be simplified. When looking at this line in isolation, then this seems kind of obvious (skipping philosophical considerations and details about engineering and API design here).

But this is usually not how the destroy function is used!
And... it may not even be how this function is supposed to be used 😬

This is related to why I think that destroyObject cannot replace destroy in general: The destroy function is also supposed to destroy all destroyable properties as well:

Owner.prototype.destroy = function () {
  // 'Recursively' destroy all owned things that
  // can be destroyed
  this._owned.destroy();
  return destroyObject(this);
}

This cannot be done in destroyObject alone, unless destroyObject does some check like this:

  for (const key in object) {
    if (typeof object[key] === "function") {
      // If the property is destroyable, then call "destroyObject" on this property as well:
      if (key === "destroy") {
        destroyObject(object[key]);
      } else {
        object[key] = throwOnDestroyed;
      }
    }
  }

(This is a sketch here, but might actually work...)

The point about the usage of destroy is related to another detail. The implementations of destroy usually end with
return destroyObject(this);

And the intended usage of destroy (as also mentioned, but not made explicit, in the coding guide) is that of
this.thing = this.thing && this.thing.destroy();
which has the effect of

1. destroying that 'thing' iff it was not undefined
2. setting that property to undefined

I've seen snippets like

  this._textureCache = this._textureCache.destroy();
  this._defaultTexture = this._defaultTexture && this._defaultTexture.destroy();

and knowing which properties even could be undefined is another question.

But some of this could be automated, similar to the draft above, with

      if (key === "destroy") {
        destroyObject(object[key]);
        delete object[key]; // Also make that property 'undefined' after destroying
      } else {
        ...
      }

One concern would be that people could add a function called destroy on their classes, and are not aware about the highly special semantics that this function would have in the destroyObject function. But maybe this could be thought through more thoroughly, and alleviated somehow.

[jjspace]

// If the property is destroyable, then call "destroyObject" on this property as well:
if (key === "destroy") {

If this is the only alternative we can come up with then I don't think it should happen. As you pointed out this opens up a lot of unknowns about properties that may or may not exist. Also the potential to call destroy on something that's in a property but not considered fully "owned" by the object being destroyed.

The destroy function is also supposed to destroy all destroyable properties as well:

Replacing the destroy function in destroyObject would not prevent this functionality from happening. Calling SomeClass.destroy() the first time would call the proper function which handles destroying all of the objects it owns and needs to destroy. Then it would call destroyObject which would replace the destroy function with a no-op meaning the next time you call SomeClass.destroy() it does nothing. But there should also be nothing it has to do anymore since it already tore down everything it should have.

[javagl]

If this is the only alternative we can come up with then I don't think it should happen.

Yes, the side-effects are hard to predict here, and that approach would require a concept of "ownership" that is much more strict than what is even possible in JS.

Replacing the destroy function in destroyObject would not prevent this functionality from happening.

I think I misunderstood one detail there.
But to summarize: The answer to the question in the title, from my perspective, is a clear "Yes!"

Generally speaking, calling "destroy" on a destroyed object should be safe and a no-op. In other languages, this is solved with some

final void destroy() {
    if (isDestroyed()) return;
    reallyDestroy(); // To be implemented by subclasses
}

But given that it's possible to juggle around function objects here (and this is already happening for all other functions anyhow), replacing destroy with () => {} would make sense IMHO.