From 40459ba29a086d34a03b26df8dab99a9e605d6b8 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 16:45:25 -0700 Subject: [PATCH 01/10] Cache redundant permission checks --- .../org/incendo/cloud/CommandManager.java | 45 ++++++++++++++++--- .../incendo/cloud/setting/ManagerSetting.java | 5 ++- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index b063582ba..2a68b5a52 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -29,10 +29,13 @@ import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; import org.apiguardian.api.API; import org.checkerframework.checker.nullness.qual.NonNull; @@ -378,6 +381,21 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { return Collections.unmodifiableSet(new HashSet<>(this.capabilities)); } + private final ThreadLocal, PermissionResult>> threadLocalPermissionCache = + ThreadLocal.withInitial(ConcurrentHashMap::new); + + @SuppressWarnings("rawtypes") + private @NonNull PermissionResult testPermissionCaching( + final @NonNull C sender, + final @NonNull T permission, + final @NonNull Function, @NonNull PermissionResult> tester + ) { + if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { + return tester.apply(Pair.of(sender, permission)); + } + return this.threadLocalPermissionCache.get().computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester); + } + /** * Checks if the command sender has the required permission and returns the result. * @@ -386,31 +404,44 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { * @return a {@link PermissionResult} representing whether the sender has the permission */ @API(status = API.Status.STABLE) - @SuppressWarnings("unchecked") public @NonNull PermissionResult testPermission( final @NonNull C sender, final @NonNull Permission permission ) { + try { + return this.testPermission_(sender, permission); + } finally { + this.threadLocalPermissionCache.get().clear(); + } + } + + @SuppressWarnings("unchecked") + private @NonNull PermissionResult testPermission_(final @NonNull C sender, final @NonNull Permission permission) { if (permission instanceof PredicatePermission) { - return ((PredicatePermission) permission).testPermission(sender); + return this.testPermissionCaching(sender, (PredicatePermission) permission, pair -> { + return pair.second().testPermission(pair.first()); + }); } else if (permission instanceof OrPermission) { for (final Permission innerPermission : permission.permissions()) { final PermissionResult result = this.testPermission(sender, innerPermission); if (result.allowed()) { - return result; // short circuit the first true result + return result; } } - return PermissionResult.denied(permission); // none returned true + return PermissionResult.denied(permission); } else if (permission instanceof AndPermission) { for (final Permission innerPermission : permission.permissions()) { final PermissionResult result = this.testPermission(sender, innerPermission); if (!result.allowed()) { - return result; // short circuit the first false result + return result; } } - return PermissionResult.allowed(permission); // all returned true + return PermissionResult.allowed(permission); } - return PermissionResult.of(permission.isEmpty() || this.hasPermission(sender, permission.permissionString()), permission); + return this.testPermissionCaching(sender, permission, pair -> { + return PermissionResult.of( + pair.second().isEmpty() || this.hasPermission(pair.first(), pair.second().permissionString()), pair.second()); + }); } /** diff --git a/cloud-core/src/main/java/org/incendo/cloud/setting/ManagerSetting.java b/cloud-core/src/main/java/org/incendo/cloud/setting/ManagerSetting.java index 55dc6ac71..04b48fc78 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/setting/ManagerSetting.java +++ b/cloud-core/src/main/java/org/incendo/cloud/setting/ManagerSetting.java @@ -69,5 +69,8 @@ public enum ManagerSetting implements Setting { * {@link org.incendo.cloud.exception.NoPermissionException}. */ @API(status = API.Status.EXPERIMENTAL) - HIDE_COMMAND_EXISTENCE + HIDE_COMMAND_EXISTENCE, + + @API(status = API.Status.EXPERIMENTAL) + REDUCE_REDUNDANT_PERMISSION_CHECKS } From 0a5a3f1c5d788eac6acf6fa84553d3905c279855 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 16:46:48 -0700 Subject: [PATCH 02/10] fix recursion --- .../src/main/java/org/incendo/cloud/CommandManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index 2a68b5a52..b65dc2900 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -423,7 +423,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { }); } else if (permission instanceof OrPermission) { for (final Permission innerPermission : permission.permissions()) { - final PermissionResult result = this.testPermission(sender, innerPermission); + final PermissionResult result = this.testPermission_(sender, innerPermission); if (result.allowed()) { return result; } @@ -431,7 +431,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { return PermissionResult.denied(permission); } else if (permission instanceof AndPermission) { for (final Permission innerPermission : permission.permissions()) { - final PermissionResult result = this.testPermission(sender, innerPermission); + final PermissionResult result = this.testPermission_(sender, innerPermission); if (!result.allowed()) { return result; } From 715d384efcc3e4219c7184715156e221a30e916c Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 16:51:09 -0700 Subject: [PATCH 03/10] fix nested testPermission calls from PredicatePermissions --- .../main/java/org/incendo/cloud/CommandManager.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index b65dc2900..f5fe7195d 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -24,8 +24,11 @@ package org.incendo.cloud; import io.leangen.geantyref.TypeToken; +import java.util.ArrayDeque; import java.util.Collection; import java.util.Collections; +import java.util.Deque; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; import java.util.List; @@ -381,8 +384,8 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { return Collections.unmodifiableSet(new HashSet<>(this.capabilities)); } - private final ThreadLocal, PermissionResult>> threadLocalPermissionCache = - ThreadLocal.withInitial(ConcurrentHashMap::new); + private final ThreadLocal, PermissionResult>>> threadLocalPermissionCache = + ThreadLocal.withInitial(ArrayDeque::new); @SuppressWarnings("rawtypes") private @NonNull PermissionResult testPermissionCaching( @@ -393,7 +396,8 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { return tester.apply(Pair.of(sender, permission)); } - return this.threadLocalPermissionCache.get().computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester); + return Objects.requireNonNull(this.threadLocalPermissionCache.get().peek()) + .computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester); } /** @@ -409,9 +413,10 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final @NonNull Permission permission ) { try { + this.threadLocalPermissionCache.get().push(new HashMap<>()); return this.testPermission_(sender, permission); } finally { - this.threadLocalPermissionCache.get().clear(); + this.threadLocalPermissionCache.get().pop(); } } From 7e3e1b912c497616e50787bb98ab8c3a78e379b6 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 16:56:40 -0700 Subject: [PATCH 04/10] reuse cache for nested calls --- .../java/org/incendo/cloud/CommandManager.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index f5fe7195d..546faac17 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -24,10 +24,8 @@ package org.incendo.cloud; import io.leangen.geantyref.TypeToken; -import java.util.ArrayDeque; import java.util.Collection; import java.util.Collections; -import java.util.Deque; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; @@ -35,7 +33,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Function; @@ -384,8 +382,8 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { return Collections.unmodifiableSet(new HashSet<>(this.capabilities)); } - private final ThreadLocal, PermissionResult>>> threadLocalPermissionCache = - ThreadLocal.withInitial(ArrayDeque::new); + private final ThreadLocal, PermissionResult>, AtomicInteger>> threadLocalPermissionCache = + ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0))); @SuppressWarnings("rawtypes") private @NonNull PermissionResult testPermissionCaching( @@ -396,7 +394,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { return tester.apply(Pair.of(sender, permission)); } - return Objects.requireNonNull(this.threadLocalPermissionCache.get().peek()) + return this.threadLocalPermissionCache.get().first() .computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester); } @@ -413,10 +411,13 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final @NonNull Permission permission ) { try { - this.threadLocalPermissionCache.get().push(new HashMap<>()); + this.threadLocalPermissionCache.get().second().incrementAndGet(); return this.testPermission_(sender, permission); } finally { - this.threadLocalPermissionCache.get().pop(); + final Pair, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get(); + if (pair.second().getAndDecrement() == 1) { + pair.first().clear(); + } } } From 0068a7f90333ec532bd54035f76ddc6cc6549b48 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 17:06:45 -0700 Subject: [PATCH 05/10] Don't create waste objects when setting is off --- .../main/java/org/incendo/cloud/CommandManager.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index 546faac17..e6c83bc21 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -411,12 +411,16 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final @NonNull Permission permission ) { try { - this.threadLocalPermissionCache.get().second().incrementAndGet(); + if (this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { + this.threadLocalPermissionCache.get().second().incrementAndGet(); + } return this.testPermission_(sender, permission); } finally { - final Pair, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get(); - if (pair.second().getAndDecrement() == 1) { - pair.first().clear(); + if (this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { + final Pair, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get(); + if (pair.second().getAndDecrement() == 1) { + pair.first().clear(); + } } } } From 9eb11d4c48702b60d9dfdfd8f3a52e2db7554401 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 17:09:14 -0700 Subject: [PATCH 06/10] Fix potential issue with toggling setting mid-permission check --- .../src/main/java/org/incendo/cloud/CommandManager.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index e6c83bc21..e0139e699 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -410,13 +410,14 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final @NonNull C sender, final @NonNull Permission permission ) { + final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS); try { - if (this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { + if (cache) { this.threadLocalPermissionCache.get().second().incrementAndGet(); } return this.testPermission_(sender, permission); } finally { - if (this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { + if (cache) { final Pair, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get(); if (pair.second().getAndDecrement() == 1) { pair.first().clear(); From 3590facce076b1956ffda08e7edbbe420dfa44ee Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 17:10:28 -0700 Subject: [PATCH 07/10] Fix potential issue with toggling setting mid-permission check (2) --- .../src/main/java/org/incendo/cloud/CommandManager.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index e0139e699..f1214c9d5 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -413,7 +413,11 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS); try { if (cache) { - this.threadLocalPermissionCache.get().second().incrementAndGet(); + final int prev = this.threadLocalPermissionCache.get().second().incrementAndGet(); + if (prev == 0) { + // Cleanup from case where cache was enabled mid-permission check + this.threadLocalPermissionCache.get().first().clear(); + } } return this.testPermission_(sender, permission); } finally { From 449b5fc2fead8d2120c73f8c178dc17a1778a70f Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Wed, 21 Feb 2024 17:10:52 -0700 Subject: [PATCH 08/10] incrementAndGet -> getAndIncrement --- cloud-core/src/main/java/org/incendo/cloud/CommandManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index f1214c9d5..92fcecbd4 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -413,7 +413,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS); try { if (cache) { - final int prev = this.threadLocalPermissionCache.get().second().incrementAndGet(); + final int prev = this.threadLocalPermissionCache.get().second().getAndIncrement(); if (prev == 0) { // Cleanup from case where cache was enabled mid-permission check this.threadLocalPermissionCache.get().first().clear(); From 1640052a16b820097f1a5712fe6cdf33fb4a3ea7 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Thu, 22 Feb 2024 15:44:46 -0700 Subject: [PATCH 09/10] Cache parent/child permission results when attempting to convert no permission to syntax error --- .../org/incendo/cloud/CommandManager.java | 45 ++-------- .../java/org/incendo/cloud/CommandTree.java | 48 +++++------ .../internal/ThreadLocalPermissionCache.java | 82 +++++++++++++++++++ 3 files changed, 112 insertions(+), 63 deletions(-) create mode 100644 cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index 92fcecbd4..7d9ed76ec 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -26,17 +26,13 @@ import io.leangen.geantyref.TypeToken; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; -import java.util.function.Function; import java.util.stream.Collectors; import org.apiguardian.api.API; import org.checkerframework.checker.nullness.qual.NonNull; @@ -66,6 +62,7 @@ import org.incendo.cloud.injection.ParameterInjectorRegistry; import org.incendo.cloud.internal.CommandNode; import org.incendo.cloud.internal.CommandRegistrationHandler; +import org.incendo.cloud.internal.ThreadLocalPermissionCache; import org.incendo.cloud.meta.CommandMeta; import org.incendo.cloud.parser.ArgumentParser; import org.incendo.cloud.parser.ParserParameter; @@ -115,6 +112,7 @@ public abstract class CommandManager implements Stateful, private final Set capabilities = new HashSet<>(); private final ExceptionController exceptionController = new ExceptionController<>(); private final CommandExecutor commandExecutor; + final ThreadLocalPermissionCache threadLocalPermissionCache = new ThreadLocalPermissionCache<>(this.settings); private CaptionFormatter captionVariableReplacementHandler = CaptionFormatter.placeholderReplacing(); private CommandSyntaxFormatter commandSyntaxFormatter = new StandardCommandSyntaxFormatter<>(this); @@ -382,22 +380,6 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { return Collections.unmodifiableSet(new HashSet<>(this.capabilities)); } - private final ThreadLocal, PermissionResult>, AtomicInteger>> threadLocalPermissionCache = - ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0))); - - @SuppressWarnings("rawtypes") - private @NonNull PermissionResult testPermissionCaching( - final @NonNull C sender, - final @NonNull T permission, - final @NonNull Function, @NonNull PermissionResult> tester - ) { - if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { - return tester.apply(Pair.of(sender, permission)); - } - return this.threadLocalPermissionCache.get().first() - .computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester); - } - /** * Checks if the command sender has the required permission and returns the result. * @@ -410,30 +392,13 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final @NonNull C sender, final @NonNull Permission permission ) { - final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS); - try { - if (cache) { - final int prev = this.threadLocalPermissionCache.get().second().getAndIncrement(); - if (prev == 0) { - // Cleanup from case where cache was enabled mid-permission check - this.threadLocalPermissionCache.get().first().clear(); - } - } - return this.testPermission_(sender, permission); - } finally { - if (cache) { - final Pair, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get(); - if (pair.second().getAndDecrement() == 1) { - pair.first().clear(); - } - } - } + return this.threadLocalPermissionCache.withPermissionCache(() -> this.testPermission_(sender, permission)); } @SuppressWarnings("unchecked") private @NonNull PermissionResult testPermission_(final @NonNull C sender, final @NonNull Permission permission) { if (permission instanceof PredicatePermission) { - return this.testPermissionCaching(sender, (PredicatePermission) permission, pair -> { + return this.threadLocalPermissionCache.testPermissionCaching(sender, (PredicatePermission) permission, pair -> { return pair.second().testPermission(pair.first()); }); } else if (permission instanceof OrPermission) { @@ -453,7 +418,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { } return PermissionResult.allowed(permission); } - return this.testPermissionCaching(sender, permission, pair -> { + return this.threadLocalPermissionCache.testPermissionCaching(sender, permission, pair -> { return PermissionResult.of( pair.second().isEmpty() || this.hasPermission(pair.first(), pair.second().permissionString()), pair.second()); }); diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java b/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java index f6b6f531a..8ece78075 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java @@ -357,31 +357,33 @@ private Exception noPermissionOrSyntax( ); } - if (this.childPermitted(root, sender)) { - return new InvalidSyntaxException( - this.commandManager.commandSyntaxFormatter().apply(sender, (List) this.getComponentChain(root), root), - sender, this.getComponentChain(root) - ); - } + return this.commandManager.threadLocalPermissionCache.withPermissionCache(() -> { + if (this.childPermitted(root, sender)) { + return new InvalidSyntaxException( + this.commandManager.commandSyntaxFormatter().apply(sender, (List) this.getComponentChain(root), root), + sender, this.getComponentChain(root) + ); + } - final @Nullable List> parentChain = this.permittedParentChain(root, sender); - if (parentChain != null) { - return new InvalidSyntaxException( - this.commandManager.commandSyntaxFormatter().apply( - sender, - parentChain.stream().map(CommandNode::component) - .filter(Objects::nonNull).collect(Collectors.toList()), - root - ), - sender, this.getComponentChain(root) - ); - } + final @Nullable List> parentChain = this.permittedParentChain(root, sender); + if (parentChain != null) { + return new InvalidSyntaxException( + this.commandManager.commandSyntaxFormatter().apply( + sender, + parentChain.stream().map(CommandNode::component) + .filter(Objects::nonNull).collect(Collectors.toList()), + root + ), + sender, this.getComponentChain(root) + ); + } - return new NoPermissionException( - permissionResult, - sender, - this.getComponentChain(root) - ); + return new NoPermissionException( + permissionResult, + sender, + this.getComponentChain(root) + ); + }); } private boolean childPermitted(final CommandNode node, final C sender) { diff --git a/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java b/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java new file mode 100644 index 000000000..80817a88d --- /dev/null +++ b/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java @@ -0,0 +1,82 @@ +// +// MIT License +// +// Copyright (c) 2024 Incendo +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. +// +package org.incendo.cloud.internal; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; +import java.util.function.Supplier; +import org.apiguardian.api.API; +import org.checkerframework.checker.nullness.qual.NonNull; +import org.incendo.cloud.permission.Permission; +import org.incendo.cloud.permission.PermissionResult; +import org.incendo.cloud.setting.Configurable; +import org.incendo.cloud.setting.ManagerSetting; +import org.incendo.cloud.type.tuple.Pair; + +@API(status = API.Status.INTERNAL) +public final class ThreadLocalPermissionCache { + private final ThreadLocal, PermissionResult>, AtomicInteger>> threadLocalPermissionCache = + ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0))); + private final Configurable settings; + + public ThreadLocalPermissionCache(final Configurable settings) { + this.settings = settings; + } + + public T withPermissionCache(final Supplier action) { + final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS); + try { + if (cache) { + final int prev = this.threadLocalPermissionCache.get().second().getAndIncrement(); + if (prev == 0) { + // Cleanup from case where cache was enabled mid-permission check + this.threadLocalPermissionCache.get().first().clear(); + } + } + return action.get(); + } finally { + if (cache) { + final Pair, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get(); + if (pair.second().getAndDecrement() == 1) { + pair.first().clear(); + } + } + } + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + public @NonNull PermissionResult testPermissionCaching( + final @NonNull C sender, + final @NonNull T permission, + final @NonNull Function, @NonNull PermissionResult> tester + ) { + if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) { + return tester.apply(Pair.of(sender, permission)); + } + return this.threadLocalPermissionCache.get().first() + .computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester); + } +} From 2a6738c820f9915a197a788d8dccb3accfd70501 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Sun, 25 Feb 2024 13:43:22 -0700 Subject: [PATCH 10/10] fix style --- .../org/incendo/cloud/CommandManager.java | 14 +++++++----- .../java/org/incendo/cloud/CommandTree.java | 2 +- .../internal/ThreadLocalPermissionCache.java | 22 +++++++++++++++++++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java index 7d9ed76ec..71a94508c 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandManager.java @@ -112,7 +112,7 @@ public abstract class CommandManager implements Stateful, private final Set capabilities = new HashSet<>(); private final ExceptionController exceptionController = new ExceptionController<>(); private final CommandExecutor commandExecutor; - final ThreadLocalPermissionCache threadLocalPermissionCache = new ThreadLocalPermissionCache<>(this.settings); + private final ThreadLocalPermissionCache threadLocalPermissionCache = new ThreadLocalPermissionCache<>(this.settings); private CaptionFormatter captionVariableReplacementHandler = CaptionFormatter.placeholderReplacing(); private CommandSyntaxFormatter commandSyntaxFormatter = new StandardCommandSyntaxFormatter<>(this); @@ -178,6 +178,10 @@ protected CommandManager( return this.commandExecutor; } + final @NonNull ThreadLocalPermissionCache threadLocalPermissionCache() { + return this.threadLocalPermissionCache; + } + /** * Returns the suggestion factory. * @@ -392,18 +396,18 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { final @NonNull C sender, final @NonNull Permission permission ) { - return this.threadLocalPermissionCache.withPermissionCache(() -> this.testPermission_(sender, permission)); + return this.threadLocalPermissionCache.withPermissionCache(() -> this.testPermissionRaw(sender, permission)); } @SuppressWarnings("unchecked") - private @NonNull PermissionResult testPermission_(final @NonNull C sender, final @NonNull Permission permission) { + private @NonNull PermissionResult testPermissionRaw(final @NonNull C sender, final @NonNull Permission permission) { if (permission instanceof PredicatePermission) { return this.threadLocalPermissionCache.testPermissionCaching(sender, (PredicatePermission) permission, pair -> { return pair.second().testPermission(pair.first()); }); } else if (permission instanceof OrPermission) { for (final Permission innerPermission : permission.permissions()) { - final PermissionResult result = this.testPermission_(sender, innerPermission); + final PermissionResult result = this.testPermissionRaw(sender, innerPermission); if (result.allowed()) { return result; } @@ -411,7 +415,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) { return PermissionResult.denied(permission); } else if (permission instanceof AndPermission) { for (final Permission innerPermission : permission.permissions()) { - final PermissionResult result = this.testPermission_(sender, innerPermission); + final PermissionResult result = this.testPermissionRaw(sender, innerPermission); if (!result.allowed()) { return result; } diff --git a/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java b/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java index 8ece78075..4c6afda77 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java +++ b/cloud-core/src/main/java/org/incendo/cloud/CommandTree.java @@ -357,7 +357,7 @@ private Exception noPermissionOrSyntax( ); } - return this.commandManager.threadLocalPermissionCache.withPermissionCache(() -> { + return this.commandManager.threadLocalPermissionCache().withPermissionCache(() -> { if (this.childPermitted(root, sender)) { return new InvalidSyntaxException( this.commandManager.commandSyntaxFormatter().apply(sender, (List) this.getComponentChain(root), root), diff --git a/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java b/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java index 80817a88d..ae73b3d97 100644 --- a/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java +++ b/cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java @@ -38,14 +38,27 @@ @API(status = API.Status.INTERNAL) public final class ThreadLocalPermissionCache { + private final ThreadLocal, PermissionResult>, AtomicInteger>> threadLocalPermissionCache = ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0))); private final Configurable settings; + /** + * Create a new cache. + * + * @param settings settings + */ public ThreadLocalPermissionCache(final Configurable settings) { this.settings = settings; } + /** + * Perform an action in a cached scope. + * + * @param action action + * @param result type + * @return result + */ public T withPermissionCache(final Supplier action) { final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS); try { @@ -67,6 +80,15 @@ public T withPermissionCache(final Supplier action) { } } + /** + * Test permission caching. + * + * @param sender sender + * @param permission permission + * @param tester tester + * @param permission type + * @return permission result + */ @SuppressWarnings({"rawtypes", "unchecked"}) public @NonNull PermissionResult testPermissionCaching( final @NonNull C sender,