Destroying triggers - where's the truth?

First of all, note that I haven't been here long enough to know if this what I'm about to say was discussed here before. So here it goes.

As you probably know, dynamic triggers (the ones that are created AND destroyed) can be a great help to developing maps and systems (triggers are destroyed to prevent action leaking) but somehow the DestroyTrigger function is avoided by experienced JASSers because of the infamous handle stack corruption bug (as posted by Strilanc here):

But e.g. DotA uses a HUGE amount of dynamic triggers (although IceFrog's style is better avoided because it involves using triggers in huge quantities instead of timers and using gamecache whereever possible) - and dota works (the crashes some versions ago were probably caused by I2H abuse).

So where's the truth?

I did some tests and figured out this:

1. How handles work (if you don't know that already): WC3 keeps a so-called handle stack and an integer containing the next unused handle value (nitially 1048577 = 2^20+1, this value belongs to some timer in blizzard.j file, if you know where the 2^20 went please tell me).
I'm not talking here about how reference counts work etc.
When you create an object WC3 first looks in the stack, if its not empty it assigns the new object to the handle in the top of the stack (the last released handle) and removes it from the stack. If the stack is empty it assigns the object to the first unused handle and increases that variable by 1.
When you release a handle (by assigning null to it), it simply is placed on top of the stack. But note that it is not added to the stack immediately but after a .0 delay.

2. How triggers work: when a trigger is activated (not by TriggerExecute/evaluate, but by an event) a handle is created. I have no idea what is stored in that handle. It gets removed after the trigger finishes execution. If you destroy the trigger during execution (in actions, removing it in conditions would crash the tread), it releases handles associated with the trigger including this handle, and after a triggersleepaction apparently wc3 sees that the trigger has finished execution (since it no longer exists) and releases this handle again, so it gets added to the triggerstack twice, and next 2 allocated objects would get the same handle.

So apparently the bug only occurs if you destroy the trigger during its execution (or probably immediately after that), but it is safe to remove it after some time.

Again, sorry if I'm repeating some other thread.

So, if we were to make a damage detection system that would recycle triggers, we would probably want to have the trigger action (or condition) to check if the trigger has more than N events and if so, run a ~0.5 second timer that would recreate the trigger. It should always be the same timer so if trigger is activated again in these 0.5 seconds the timer gets restarted. This would eliminate the possiblity of destroying a trigger right after it finished execution.

I already told you, if you don't use Actions you can create and destroy triggers as much as you like.

The thing is that if you don't use actions then you cannot use TriggerSleepAction() and since TriggerSleepCondition() does not exist there is no way to create a bug.

The only reason dota does not crash all the time is because it has no damage detection system.

It used to crash very often, like once a day if you play it often enough.

Can anyone reproduce a bug with actions+destroytrigger without destroying trigger from within the action? Or at least without using waits.

And does anyone know what that handle is for (the one that is created on trigger execution)?

So I guess using all-conditions and not destroying triggers carelessly would be safe (oh, wait, instead of making that stupid resetting timer you can disable the trigger and destroy it in a minute or so).

Your explanation can't be right, although it's probably close. In the example code the double-free trigger hasn't finished executing when the check is done, so it must be more than 'finished execution' that is double-stacking.

Here is what I think is happening:
Blizzard realized you might destroy the triggering trigger, and tried to compensate for it. I assume there's a flag that is set when the executing trigger is destroyed. But they did a half-assed job, so the flag isn't preserved when a destroyed trigger resumes execution.

End result: the first wait is handled correctly, but the second wait isn't. Actually, based on this explanation I would expect there to be another copy of the handle index added to the stack when the trigger ends execution. Someone should check that.

proof by popular map is getting very lame, could people please stop doing it?

Either way, IceFrog is the one who actually found the smoking DestroyTrigger gun.

--
Not using waits will prevent you from having problems related to the easy to find DestroyTrigger bug.

I have no idea if using Conditions helps at all, it could be that just not letting you to use waits does prevent that bug I just mentioned. Something to notice is that you indeed can use waits inside a condition, I do not know if those waits would still have the easy.

However, the other bug with DestroyTrigger, it is either avoided by not using DestroyTrigger or I2H or both.


Anyway, the only application left for dynamic triggers is... damage detection? Even then leaking the events seems like a lesser evil. Really, dynamic triggers are worthless. Even if they didn't create issues they would still make your code more complicated and force you to use attaching when you do not need it.

Perhaps, there are ways to use I2H and DestroyTrigger and have no issues, in my opinion both things are entirely useless and are not worth the effort of doing trial and error until you find it, or to expect you to be lucky and have no issues with them in a map (or no users reporting them whatsoever) so you can call yourself a experienced jasser.

So, a single trigger with an event added to it for every unit as it enters the map is the most advised way to do damage detection these days?

My advice is not to use damage detection, the whole deal about a single trigger for a lot of specific units is... If you have few units in your map (like 1000 units created in total) it should be ok to leak the events, else you can use different generations something that Strilanc did some day and I think Griffen was doing something like that the other day. Minimizing the calls to DestroyTrigger to one per 10 minutes should work just fine. But again, I have no idea.

There is one more option - do not allow units on your map die.

This option will work for any AOS like map, when units of same type created constantly.

Also you can play with chaos and swap unit types as many times as you like, but no one know will this work without some "hidden" problems.

The only issue with that is that you'll have to forget about spells like raise dead and every soldier needs to leave a corpse, or you can also probably fake the whole death deal using animations and pause, but I think moving dead units and using reincarnation to revive them is easier?

The system vex is talking about is SUEL. I asked for it to be moved to the graveyard because this bug could potentially break it.

Good news, though. I have come up with a way to safely destroy private triggers with unknown actions being added. [The specific case I had to deal with in SUEL]

It turns out that if you add multiple actions to a trigger, waiting in the early actions will delay the later actions. Actions are run in order, and they don't pass each other. So all I need to do is make sure the first action in a increments the trigger's 'running' count, and the last action decrements it. Then I destroy the trigger normally if it is not running, otherwise I mark it as expired and destroy it after the last run ends.

That will make SUEL totally safe and leak-free, so I might just resubmit it once I do that. On the other hand, it's overhead I wanted to avoid... we could really use a SafeDestroyTrigger function, but I'm not sure how you would go about writing something like that.

Why wouldn't a function that disables a trigger and destroys it after ~1 minute be safe?

Each event is ~400 bytes so its not fatal to leak them but it would still be good to avoid it.

And there definetly is some handle which I can't account for (e.g. 1048667 is trigger, 1048668 is event, 1048669 is action, then when trigger is activated next handle is 1048671. After destroying trigger and using 2 waits, we get two 1048670 handles). And the handle is gone after the trigger has finished execution.

Btw conditions not only stop you from using waits, but they also don't let you destroy the triggering trigger.

Probably the best thing to do is create a SafeDestroyTrigger that would do something like disable+destroy after some time, and later if that doesn't work you can make that function just disable the trigger.

I totally agree with this above all else and have said that for awhile now. What pisses me off is that it takes Vex to say it before anyone listens to it.

Besides, it's ridiculous to 'solve' the problem with DestroyTrigger by disabling it and destroying it later. That's like saying "We have no idea what's going on, so we're going to wait awhile and hope if we do this later on it won't be a problem." A more elegant solution is to just never use DestroyTrigger, since outside of that damage detection crap it has zero value anyways.

Avoiding the problem isn't nearly as interesting as trying to solve it.

I wrote something to print off the top handle indexes then put them back in correct order, and checked it at various stages of the bug happening. After the first wait the stack goes from 114...130 to 113...97, which makes no sense at all.

That's where system makers divert from mapmakers. I'd rather avoid problems in my maps than try to solve them once they arise.

Does someone know how events are attached to a trigger?
I suppose they are actually attached to the object they are related to, and if so then obviously tons events wouldn't slow anything down. Then gogo simple damage detection systems with no recycling, 400 bytes per unit is not a waste.
Can anyone with experience of coding a large RPG map (either singleplayer or multiplayer) give an estimate of the number of units that were ever created (non-dummy) in ~3 hours?