LuckPerms

LuckPerms

41.4k Downloads

Delayed permission changes incompatible with some plugins

Sir-Will opened this issue ยท 9 comments

commented

I have a plugin which adds a permission to the user and does react to the permission change instantly.
With zPerms this worked without any issues but LuckPerms seems to have a delay.

So, when the plugin looksup the users permissions LuckPerms returns the "old" information as it hasn't processed the permission changes. This is causing the plugin to not work at all.

commented

Which plugin is it?

commented

CommandHelper, ServerSigns, ChestCommands, all this plugins where you can run multiple commands.

commented

Can you explain what the issue is with that?

Like, I assume they're adding permissions to users when certain actions take place? I don't understand the problem here.

commented

ChestGUI display based on permissions.

1

  1. chest gui opens with settings and their state is based on permissions
  2. player clicks an item to change the permission which updates the permission and reloads the chest gui
  3. chest gui still displays the option like it wasn't changed (hasPermission still reports the old state)

2

  1. player clicks a sign which gives him a temporary permission and asks him to click again to confirm
  2. he clicks again and the permission isn't live yet, hence it fails

3

  1. on login (highest priority) a permission is being set
  2. on login (normal priority) another plugin is checking for this permission but its reporting the old state

.. the list goes on

For us this caused multiple systems to not work anymore and some of them can't be tweaked easily.

commented

I spoke briefly to you on Discord earlier. Just going to repeat a bit of what was said there for reference on this ticket.

The core issue is that modifications take place asynchronously. When you execute a command to modify user/group state, that change will apply at some point in the future. It is not guaranteed to be immediate.

Reasons for this:

  1. Modifying state takes time. It affects overall server performance if the main server thread stops and waits for these changes to be applied.
  2. The changes take even longer in LuckPerms than they would in other plugins, due to the way I've designed it. It's not simply a case of removing a node from a Map/Set, I have to apply the changes to multiple caching layers, and replicate the change in state across all affected objects. For example, if a group changes, I have to replicate that change across all users/groups who inherit the change.

A lot of the LuckPerms design centers around pre-processing. It means that making changes to the data is time consuming, but actually reading it is very quick.

So, why not just make the change happen synchronously like other plugins?

  1. Simply put, it's slow.
  2. I want to modify the most up-to-date object state. This means loading data from storage (SQL queries are even slower)
  3. Using this approach, "pre-processing" as I call it does far more harm than good.

So, the solution to your problem, well, I'm not really sure.

As I said on Discord, it's an implementation detail. It's not a requirement that all commands that modify data state should make the change in sync with the server thread.

In my own projects, (I write and work on multiple projects that use LuckPerms as a base for saving data about players), I simply embrace the multithreaded nature of the plugin and use callbacks. I appreciate this may not be possible when using pre-made plugins, but I really can't think of an easy way around the issue.

In reply to the individual issues above (I understand this is probably really unhelpful advice)

1 - Perhaps just refresh at a delay? Obviously you're using ChestCommands, so there's not really a simple solution. I've written similar systems using LP as a base, and have just made the inventory change whilst the permission changes go through in the background. Obviously this won't work if you're using permission state to re-build the menu. I don't really have any ideas.

2 - Not really sure with this one. But the reply is going to be the same as No. 3. If you're just wanted to set transient permissions (those that will only exist for the remainder of the session), you can just make this change through Bukkit's API. Vault has methods for it too.

I'd appreciate any other suggestions, but at this point, completely changing the system to allow for synchronised writes isn't possible. The performance benefits hugely outweigh the concurrency issues.

commented

Would be a second command which doesn't run it async a solution? So that command can be used for special cases like this.

commented

Depends if you could deal with the whole server halting for x ms every time a command runs. (basically no it's not really a solution :/ )

I don't really know what to say. Every point you make (not to say that they're invalid) was answered in the very post they were in response to.

Performance is important, and this is an edge case. In fact, I would go as far as to say it is bad practice to programmatically use the commands as an interface to change permissions data. You should either be using something like Vault or hooking with the API directly.

A better example of this:

player.performCommand("spawn");
player.teleport(spawnLocation);

The second is clearly superior, and this is just with an action that moves a player. Now consider the complexity behind modifying permissions data.

Ultimately, I do not consider the current behaviour to be a bug. It was my intention that commands would run asynchronously, and that changes would be applied at some point in the future. If you want immediate changes, you can modify Permissible state, as previously mentioned.

I am fully aware that other permission plugins run commands on the main thread, yes, I know it's possible, but my aims for this project aren't to just re-create what's been done before. I can't push limits in terms of features and performance (my core goals with this whole thing) by just rehashing old ideas and copying existing design.

There are ways around it, but it's ultimately going to fall down to you to update your systems to not depend on just executing commands.

commented

Yes, I totally agree, resolving this requires major core changes and some sacrifice of performance.

I guess the main question is, how could it be designed to be unnoticeable in terms of performance and resolve the issue.

I believe the best way would be to split the system. Into a consistent local environment while keeping the efficient backend. I'm certain it is possible for the system to perform extremely well with a local consistency when designed nicely. Frankly if the last nano second of performance is valued highly, it could be optional to turn it off and sacrifice the consistency for improved performance where necessary.
Overall I believe that this performance impact is mainly interesting for huge servers with thousands of permission changes every second.

You voiced the concern on discord that it probably wouldn't take long to update a permission which is directly attached to a player but updating a group with 50 cached players might take some time. Well I guess we would have to actually measure this to know the real impact. But it is safe to say that player edits are happening way more often than group edits. To be honest, how often do we update permissions of a group? If it is once every 20 minutes, an impact of 2ms, shouldn't matter.
Especially in comparison with the major lag spikes caused by the issue explained in #87

commented

How could you make a plugin like server signs/chest gui work? It is all about commands.