Add a mechanism so gadgetHandler reordering api can be used safely.

[saurtron]

Work done

• Create an internal mechanism at gadgets.lua to queue methods with callin list reordering side effects.
• Users of the methods shouldn't notice any difference and this shouldn't break any existing gadgets.

Addresses Issue(s)

• Dangerous RemoveGadget/RemoveWidget usage #3900

Details

NOTE This is changing critical game code so will need some review and testing. I am posting this PR to gather feedback while I can perform some last checks on the operation (like reviewing whether after initialization ordering is still the same for example). Also probably need to add some safeguards against duplicated items in the queue.

This changes all of the following callins:

• InsertGadget, RemoveGadget, EnableGadget, DisableGadget, LowerGadget, RaiseGadget, UpdateGadgetCallIn, RemoveGadgetCallIn

After reviewing the code I think all those methods can cause unsafe reordering of the callin lists (ie the problem is not just InsertGadget and RemoveGadget), while the callers are iterating them, thus can result in skipped calls.

The solution I propose consists in replacing the callins by queued versions, and providing the old api under ..'Raw' name (not to be used directly). The main handler is augmented to call a reordering method (gadgetHandler:PerformReorders) after callin loops, that will process all methods above in order after the loop is finished.

This solution should make all of the above methods safe to use during execution of any handlers. I tried to make it as efficient as possible, so in the general case after every callin loop just a local bool check is introduced before doing any heavy work.

This could result in slightly different behaviour than before in the case a gadget is removing another one before that one executes. I will also study this closely, but that might be more of a problem with widgets.

The solution can be implemented for widgets as well since handling is very similar, but I think gadgets have a higher requirement for robustness so I implemented it here first.

Test steps

• Fetch branch and run BAR

[WatchTheFort]

Why do the raw methods need to be exposed in the gadgetHandler global at all? Can't they be made private, and only called by the public functions?

[saurtron]

Why do the raw methods need to be exposed in the gadgetHandler global at all? Can't they be made private, and only called by the public functions?

Yes, they can be made private, they're just the old methods renamed. For now I changed the minimal amount of code to make changes easier to review, but they can be converted into private local methods to the module.

To be honest making them private will be a bit of pain since now they are always accessed through gadgethandler, also, most gadgets don't have access unless declaring 'handler' in the gadgetInfo structure, but it can be done for sure.

[sprunk]

Doesn't this still fail with recursive calls?

[saurtron]

Doesn't this still fail with recursive calls?

I think it should work, check this, as long as the callstack is not exhausted it should be fine.

edit: sorry, I thought you mean a reorder that triggers another reorder, if you mean reentrant callins then yes I need to account for that, I'll add a counter to make sure to only reorder after after "main" callin loop.

[sprunk]

Yes, I mean one callin being called from within another, triggering a reorder while the outer loop is stilliterating. Solving it via depth counter sounds good.

[sprunk]

Another remark: widget handler will need the same treatment as gadget handler

[saurtron]

Another remark: widget handler will need the same treatment as gadget handler

Indeed. Once this is done I intend to implement for widgets too, just wanted to ensure the approach is wanted before doing so, also limiting the PR scope that way.

[saurtron]

Tested before and after ordering (printing all synced and unsynced gadgets at the end of gadgetHandler:Initialize) and I can confirm indeed the ordering doesn't change.

Procedure used for test:

1. Added the following snippet at the end of gadgetHandler:Initialize, Both for master and rebased safe-ordering branches:

       Spring.Echo("ORDERING")
       for _, g in pairs(self.gadgets) do
               Spring.Echo(g.ghInfo.basename, g.ghInfo.layer)
       end
       Spring.Echo("END ORDERING")

2. Manually cut everything not between ORDERING and END ORDERING at infolog.txt (appears two times due to synced and unsynced paths). Save in two txt files file1.txt and file2.txt.

3. Run cut -d' ' -f2- file1.txt > f1.txt and cut -d' ' -f2- file2.txt > f2.txt to remove the timestamps.

4. diff f1.txt f2.txt -> yields empty result

Resulting files: f1.txt, f2.txt (yes they look like the same file)