search

WeakAuras / WeakAuras2

World of Warcraft addon that provides a powerful framework to display customizable graphics on your screen.
https://weakauras.wtf
GNU General Public License v2.0
1.32k stars 318 forks source link

defer calls to ScanEvents & friends if it doesn't come from WeakAuras #5555

Open emptyrivers opened 3 days ago

emptyrivers commented 3 days ago

Primarily this ensures that we don't have any kind of reentrancy, which has the potential to break invariants.

emptyrivers commented 3 days ago

I believe that with this change, there's also some machinery in AuraEnvironment which is unnecessary (because its purpose is to defend against exactly the reentrancy which is now impossible)

InfusOnWoW commented 3 days ago

The code looks good, though I haven't tested it against the ticket to check if it solves that problem.

I have two potential concerns: a) Some external addons call WeakAuras.ScanEvents(). Should that affect how we do this. b) A potential other solution would be to have the same "deferred queue", but that the handling of that queue is done at the end of all code paths that can lead to the queue getting filled.

The issue in the linked ticket was iirc from an every frame update of an text. Having the deferred handling via a separate frame means that the ScanEvents potentially happens at the next frame, depending on how wow iterates the frames for their OnUpdate handler. I would suspect that some people rely on the same frame handling that is currently always the case.

emptyrivers commented 2 days ago

a) Some external addons call WeakAuras.ScanEvents(). Should that affect how we do this.

I did think about that. I suspect in most cases those addons are using WeakAuras as a "frontend" display and aren't expecting any kind of backflow of data or control. For those cases the point is moot, and for the rest, we could commit to the async bit and allow callers to register a post-callback once we process the event. But that seems niche enough that i'd like to see a real use case before implementing it.

emptyrivers commented 2 days ago

b) A potential other solution would be to have the same "deferred queue", but that the handling of that queue is done at the end of all code paths that can lead to the queue getting filled.

That would work but it will lead to at least one stupid bug in the future because we forgot about deferred events when implementing something new. The benefits aren't obvious enough for me to want to point a gun at my foot, but i could be convinced.

InfusOnWoW commented 23 hours ago

I think I'm fine with it as is. I'll do a quick test against the actual problematic aura, and merge it then.