Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure to remove perm/op after command #1277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Phoenix616
Copy link
Contributor

This change makes sure that the wildcard permission and op status are properly removed from the player after the command in the command items or custom crafting configs are executed even if any kind of exception occurred while executing it.

Also adjusted the CustomCrafting op setters to work the same way as the CommandItems ones (checking the wasOp) and explicitly setting the op to false after execution to improve code readability.

@me4502
Copy link
Member

me4502 commented Jul 22, 2021

The reason this isn't done is that it was causing server deadlocks in some cases.

Generally the SUPERUSER commanditem run as type is no longer supported, and will be fully removed in CB5.

It may be replaced with a permission attachment system, but generally that doesn't work too well with how perm plugins do caching now. Console is the recommended way

@Phoenix616
Copy link
Contributor Author

Tbh. I feel like a deadlock would still be a lot better than a player walking around with op and * permission.

@me4502
Copy link
Member

me4502 commented Jul 22, 2021

The deadlock was much more likely than the case you described. Really I'd prefer to keep it in the "not recommended" state as is with it /mostly/ working, then remove it in CB5 where people won't send me death threats for months for removing a feature in a non-major release

@me4502
Copy link
Member

me4502 commented Jul 22, 2021

To clarify - changing it to this makes the feature detrimental enough that it's /likely/ to cause problems, which massively increases the current risk factor. I'd rather entirely remove it than introduce the deadlock issue

@Phoenix616
Copy link
Contributor Author

Do you have any further information on the deadlock? Because I have been running a similar code for years without any issues...

@me4502
Copy link
Member

me4502 commented Jul 22, 2021

Not really - it was just something that got reported a lot without reproducibility on my end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants