[Scarpet] There is an abnormal amount of callbacks actually called for custom events, increasing quadratically
altrisi opened this issue ยท 17 comments
So this was initially going to be an issue about having two (actually == player count) non-player instance of player-scoped apps instead of one (please tell me it's intended to have a (somewhat accessible) instance "not tied to a player" in player scoped apps, it's quite useful), but ended up being a bug report about the number of player-scoped instances for an app's custom callbacks being ridiculous.
For example, I just summoned 34 players in a single player world (including me) and there are 1260 instances of callbacks running (34 of those not being tied to a player I think, but that's no longer this issue).
Steps to reproduce:
- Put this in an app:
handle_event('test_event', _(data) -> (
logger('info',data); //# Or something that doesn't spam
);
);
- Spawn a few players (useful command:
/script run for(range(0,40),run('player Player'+_+' spawn'))
to spawn 40 players) - Run
signal_event('test_event', 'player', 'hi')
('player'
can benull
instead) - Enjoy 1722 calls logging
data
and potentially causing other side effects in apps
Haven't really checked whether it also happens with built-in events or only with handle_event
, but it doesn't seem tied to signal_event
.
Further testing seems to find that some of those callbacks are not removed after the disconnection of those players: Signaling the event again after disconnecting all the players returns 42 callbacks (there were a total of 41 players, now one. Expected 2 callbacks, not tied to player and mine).
Those that are kept seem to be tied to player name and not instance, since after reconnecting and disconnecting the same names again they don't increase, but after connecting and disconnecting different ones they do.
1722 = 41^2 + 41
and 1260 = 35^2 + 35
, so there is likely some quadratic scaling here.
Can you confirm this, for example by doing the following?
- Make sure that there are exactly 100 (
x
) players online at once. You may need to reduce render distance and/or run this on a dedicated server with all players in the same chunk and a low view-distance. - Signal the event.
- Notice that you get 10100 (
x^2 + x
==x(x+1)
) callbacks instead of the expected 100 (x
).
Yes. (also render distance doesn't really matter since all players spawn in the same position, so no extra loaded chunks)
While this IS not 100% correct behaviour, you are describing a perfectly valid usecase.
In your case there is N player-scoped apps (its a player scoped app), each handling 'hi' event. if you trigger 'hi' event (all events are fully global, not tied to any app even), then all N copies of the players will receive it, and since the broadcast doesn't have a target, its targeting every player on behalf of every player, so you would expect N*N messages. signal(event, null)
means trigger for all existing players event from all apps that handle them.
Problem is that currently also the 'server' app (within the player app) is also triggered, which is incorrect, as their events should be muted.
Consider the following app instead. "on_start" is handled properly for global and player apps, much better than static code, so it doesn't have the misfired events for server instance of a player scoped app.
__on_start() ->
(
p = player();
handler = ( _(data, outer(p)) -> logger('info', p+' says '+data+' to '+player()) );
handle_event('test_event_from_'+p, handler);
handle_event('test_event', handler);
)
Then (with 4 players)
/script run signal_event('test_event', null, 'hi')
16
/script run signal_event('test_event_from_Steve', null, 'hi')
4
I actually use on_start in some of my apps to remove handlers from the player-scoped instances, but I thought there was only one.
However, I don't fully understand why this should give N*N callbacks, I'd expect it to be called once per player instance and one per non-player instance.
Oh! I thought specifying a player would only call the event for that player instance, maybe it's that what I'm missing?
Oh, actually you mean each player instance is "redistributing" the event to every player again?
I'm not sure whether this is what you mean, since it doesn't seem expected/correct to me.
currently, if you address a specific event, in a multi-player player scoped app, you will get NN notifications (each player app will handle that event separately, and since it doesn't have the receipient, will send to all player app instances. That might not be super useful tbh. Also currently if you target a specific player, only that player app will be triggered, and will only send that event to itself (double checking for event owner), giving one call (11), which is weird as well.
To recap, currently (assuming 4 players):
- global app, event targets a player: 0 triggers (seems correct and intentional)
- global app, no target: 1 trigger (make sense as well)
- player scoped app, event targets player: 1 event fired (should it be 4 then, because each player's instance declares handling that event and sends it to the targeted player, so 4)
- player app, no signal target: 16 triggers (either is correct, or if previous is at 1, then all player instances should receive that event and only target themselves - also 4 calls, but all executed separately for each player, not one player app and executed 4 times)
I think I'm still missing something, but here is what I expected:
PS: I now found that the docs didn't mention about a 'player'
literal but a player
variable, and found that 'player'
doesn't do anything special, therefore changed the thing below and it makes a bit less sense, though with the changes it seems ok.
signal_event(ev, null, ...)
: Would signal all global instances of apps, including- Global scoped apps
- Global/static/whatever instance(s) of player-scoped apps (since, anyway, if that scope is handling events, it's usually intended, and it's a nice way to "control" a player-less player-scoped app (is it supposed to be one in total or one per player btw? I expected one in total, but it may not be that way? (from probably very very flawed empirical testing without really checking the code, which I think I remember only holding one back in the day))
signal_event(ev, <player>, ...)
Would signal instances of player-scoped apps bound to the specified<player>
oncesignal_event(ev, something_specific, ...)
Would signal every instance of all player-scoped apps, once each- Maybe
something_specific
could just be a list of all players to signal everyone, thereforesignal_event
accepting a list of players to signal the event to. However, then you would need two calls to also signal it to global scoped apps if you want to. This is the one that I thought worked with the literal'player'
, because of not reading the docs properly/too fast
- Maybe
I read the docs and it is clear I wrote them in the middle of the night. You just repeated what the docs are saying and the actual behaviour of event dissemination doesn't really work this way.
But even that some of the things mentioned there don't make sense specifically targetting signal_event(ev, null, ..) to static server instances of the player scoped apps - these are just templates for players that haven't logged in yet and events should not be handled by them at all.
What I am proposing:
signal_event(ev,
`, ...)- global apps - doesn't trigger at all, there is no way to pass target player argument
- player apps - trigger once for specified player
- `signal_event(ev, null, ...)
- global apps - trigger once global instance
- player apps - trigger N times once for each player
I think that makes most sense. That should work with built-in events, albeit here there are three tiers of events
- player targeted events (like __on_player_breaks_block)
- global apps - triggered once, providing player argument as first arguments
- player apps - triggered once, targetting given player instance, providing player argument for consistency, since
player()
returns the same always
- global events that could be handled by multiple players (__on_explosion):
- global apps - triggered once
- player apps - triggered N times for each player separately, so they can do something with them
- global events that should not be handled multiple times (its only a business decision, given typical usecase, think entity_load_handler ):
- global apps - triggered once
- player apps - not triggered / error issued
^ This comment is not in vain - once we settle on something that I would copy that to the docs
Also question is should things like entity_load_handler be handled by player scoped apps - even if it makes no sense and could be dangerous?
I'd love to have a way to target the global/static instance of player-scoped apps, but I understand if you don't want them to be accessed (though I think it's quite unlikely someone specifically handle_event
s in the static instance AND signals the global instance for it unwillingly, and I find it quite useful to have an instance I can rely on existing to put more "global" processing/storing in that can be modified through events (events because it's the current only way I mean), here assuming __on
doesn't register for such global/static instance).
About the entity load handler: I can think of a few ways to integrate it with a player-scoped app, but I think it definitely makes more sense to only allow it in a global one. If someone has a good use case for it that can be changed at a later date.
Also assuming global events for player apps you mean once per player and not N times per player, I think it makes sense.
Since memory spaces for all player scoped apps are separate, having access to the server instance and be able to do stuff with it means simply you might as well run a copy of the app specifically in the global scope. I think the concept of player scoped apps is very useful - saves a lot of coding hassle to separate players and allow easily to run several actions from different instances independently. allowing server instance to run could make it confusing when you want to target some player but do that incorrectly, and no error is thrown (for example using /execute at, not /execute as)