Permissions!

[lucko]

My aims with this were, in no particular order:

• Easy/simple to use
• Straight forward to implement, delegating all handling to the plugin managing permissions
• Fast in terms of performance

I've borrowed a few concepts from other areas of the MC ecosystem, namely

• Tristate from Sponge & LuckPerms.

• PermissionSubject from Sponge, although only the name is copied. The interface is really just a simple version of Bukkit's Permissible.

• PermissionFunction fulfils the same purpose as Bukkit's PermissibleBase, except it's not an implementation. It allows other plugins to effectively implement a PermissionSubject's hasPermission method.

Design choices:

• Why not just an event? Well, mainly performance reasons. Permission checks should be fast, and creating, scheduling and then joining a future isn't exactly fast, especially in comparison with just doing a direct call to the provider. Additionally, doing things this way allows providers to store a players permissions data directly on the provider instance, as opposed to doing map lookups.

Potential issues:

• If support for reloading individual plugins was ever implemented, we'd probably have to re-think the way plugin provided "services" are handled.

[astei]

I prefer if this did not use .join(), this blocks the Netty I/O threads and defeats the point of moving event listeners off the main thread (which was specifically intended to avoid executing or waiting on "alien" code).

[lucko]

I couldn't find a saner alternative.

The best alternative I can think of is storing the future and then calling #join during the hasPermission call? That's pretty messy though.

For events that should be returning in constant time, (i.e. a few map lookups & perhaps one new instance creation), is calling join really an issue?

Any other ideas?

[astei]

I don't doubt that LuckPerms would sanely handle this event. But I'm not so sure about other plugins, which might do things like run a SQL query when this event is fired, which was a major consideration in forcing all events to be run "asynchronously" in Velocity. It's still early days, so we could have a discussion to revisit this decision (perhaps there are valid performance considerations).

The way I see it: Velocity should do its very best to ensure the Netty I/O threads are runnable at all times doing useful work like processing packets. Using join() here defeats the point: it's blocking waiting on alien code to finish up.

[lucko]

I understand the aim, but as I said, I don't see a sane way to achieve it.

1. Call the event and store a CompletableFuture<PermissionsSetupEvent> instance on the ConnectedPlayer. I dislike this because it's messy, and you're really just moving the problem elsewhere - we'll have to block when hasPermission is called, and then also cache the creation of the resultant PermissionFunction.

2. Init the function lazily when the future completes - causes race conditions, not really an acceptable solution imo.

[astei]

Or simply initialize an UNDEFINED-only permission function in ConnectedPlayer, fire the event once ConnectedPlayer is constructed, wait for it to complete asynchronously, set the new permission function, and allow the normal login process to complete.

This should be ok: plugins handling permission setup should not rely on the player having permissions already.

[lucko]

And if the normal login process completes before permissions are loaded?

As you said:

But I'm not so sure about other plugins, which might do things like run a SQL query when this event is fired

Which:

causes race conditions, not really an acceptable solution imo

[astei]

And if the normal login process completes before permissions are loaded?

I did not mean "the normal login process completes concurrently with the event execution". Simply use whenCompleteAsync to get the execution back on the Netty event loop and finish the login process there.

[lucko]

Okay, I've moved the loading into the LoginSessionHandler, so everything is loading off the netty threads now. :)

[astei]

I am fine with the general approach of this PR. Velocity's event invocations are relatively expensive (for a good reason, in my opinion), and I believe that having the permissions on the player instance and available for quick retrieval is important. There is one other change I'd make that I've outlined, but LGTM otherwise.

[astei]

LGTM.