Explosions dont fire the Destroying event when getting cleaned up

Bug Reports Studio Bugs
#1

Reproduction steps:

  1. Run this piece of code either on server or client:
local explosion = Instance.new("Explosion", workspace)

explosion.Destroying:Connect(function()
	print("Destroyed")
end)

explosion:GetPropertyChangedSignal("Parent"):Connect(function()
	print(explosion.Parent)
end)

Behavior:

It is to be expected that both those events fire, but only the GetPropertyChangedSignal with a nil print does, which means that Explosions dont fire the Destroying signal which could be inconvenient.

3 Likes
#2

We’ve filed a ticket into our internal database for this issue, and we will update you when we have further information!

Thanks for the report!

4 Likes
#3

This is working as intended for now.

Explosions and other automatic part removing functionality in the engine just set Parent = nil, not Destroy. Contrary to the name, FallenPartsDestroyHeight also has only ever set part.Parent = nil.

Part of this is legacy, but changing this would be a breaking change, since Destroy is irreversible and locks Parent. Any script doing automatic recovery of fallen parts or pooling of Explosion instances would break if we changed this to Destroy.

We could change this, but it would be a feature request. We would need to add analytics to see how often people are re-parenting these instances after removal and likely require another three-phase rollout with a workspace Default/Enabled/Disabled enum.

...DestroyHeight not destroying comes up multiple times a year, so we probably have done that investigation before at some point, but would have to check…

5 Likes
#4

I feel like they should be destroyed, setting parent to nil only causes server/client memory to skyrocket if you heavily rely on FallenPartsDestroyHeight

3 Likes
#5

Parenting to nil doesn’t necessarily mean memory leaks, just as Destroy() doesn’t necessarily mean no memory leaks.

Destroy() parents to nil, locks the parent, and cleans up event connections. Remove() just parents to nil, but if you do not have any event connections on it or its children (you’re parenting stuff to Explosion?), then it will be the same.

Code like this for example:

rocket.Touched:Connect(function()
    local explosion = Instance.new("Explosion")
    explosion.Position = rocket.Position
    explosion.Parent = Workspace
    rocket:Destroy()
end)

…will not cause any memory leaks with explosion, as nothing references it anymore.

4 Likes
#6

What about this?

local function hit(part)
    part.BrickColor = BrickColor.random()
end

rocket.Touched:Connect(function()
    local explosion = Instance.new("Explosion")
    explosion.Position = rocket.Position
    explosion.Hit:Connect(hit)
    explosion.Parent = Workspace

    rocket:Destroy()
end)

This might leak, and there’s not a easy way to clean up the connections

3 Likes
#7

Yes, that would leak. You do need to call Destroy() in that case. I am not saying there is no issue, just that it does not necessarily “cause memory to skyrocket”.

I would file a feature request to make explosions Destroy.

3 Likes
#8

Why would that leak? Doesn’t loosing reference to rocket and explosion eventually cleans everything up?

3 Likes
#9

The Hit is not disconnected, and as per the thread explosions do not destroy (and thus do not clear event connections)–that is a reference.

4 Likes
#11

Event connections in garbage collected languages has to be my favorite example of accidental / hidden complexity.

Writing memory safe code in an unsafe language is hard. Writing event heavy code that never leaks has to be at least equally hard. The penalty for failure is still death for both, it’s just a question of how violently and rapidly death occurs.

Definitely harder to reason about and diagnose. For every connection I must consider if and when I need to disconnect it, assuming the script context won’t be shut down at that time… That probably means I need to connect to more events to disconnect the original events, and then think about when to disconnect THOSE.

5 Likes