Skip to content

Commit

Permalink
Fix default variables concurrency issues (#6996)
Browse files Browse the repository at this point in the history
* Ensure synchronization for default variables hints access

* Use Guava synchronized deque
  • Loading branch information
APickledWalrus authored Aug 31, 2024
1 parent 56119b3 commit 37449b1
Showing 1 changed file with 21 additions and 9 deletions.
30 changes: 21 additions & 9 deletions src/main/java/ch/njol/skript/structures/StructVariables.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.Locale;
import java.util.Map;

import com.google.common.collect.Queues;
import org.bukkit.event.Event;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.Unmodifiable;
Expand Down Expand Up @@ -85,7 +86,16 @@ public class StructVariables extends Structure {

public static class DefaultVariables implements ScriptData {

private final Deque<Map<String, Class<?>[]>> hints = new ArrayDeque<>();
/*
* Performance/Risk Notice:
* In the event that an element is pushed to the deque on one thread, causing it to grow, a second thread
* waiting to access the dequeue may not see the correct deque or pointers (the backing array is not volatile),
* causing issues such as a loss of data or attempting to write beyond the array's capacity.
* It is unlikely for the array to ever grow from its default capacity (16), as this would require extreme
* nesting of variables (e.g. {a::%{b::%{c::<and so on>}%}%} (given the current usage of enter/exit scope)
* While thread-safe deque implementations are available, this setup has been chosen for performance.
*/
private final Deque<Map<String, Class<?>[]>> hints = Queues.synchronizedDeque(new ArrayDeque<>());
private final List<NonNullPair<String, Object>> variables;
private boolean loaded;

Expand All @@ -99,9 +109,10 @@ public void add(String variable, Class<?>... hints) {
if (CollectionUtils.containsAll(hints, Object.class)) // Ignore useless type hint.
return;
// This important empty check ensures that the variable type hint came from a defined DefaultVariable.
if (this.hints.isEmpty())
Map<String, Class<?>[]> map = this.hints.peekFirst();
if (map == null)
return;
this.hints.getFirst().put(variable, hints);
map.put(variable, hints);
}

public void enterScope() {
Expand All @@ -119,12 +130,13 @@ public void exitScope() {
* @param variable The variable string of a variable.
* @return type hints of a variable if found otherwise null.
*/
@Nullable
public Class<?>[] get(String variable) {
for (Map<String, Class<?>[]> map : hints) {
Class<?>[] hints = map.get(variable);
if (hints != null && hints.length > 0)
return hints;
public Class<?> @Nullable [] get(String variable) {
synchronized (hints) { // must manually synchronize for iterators
for (Map<String, Class<?>[]> map : hints) {
Class<?>[] hints = map.get(variable);
if (hints != null && hints.length > 0)
return hints;
}
}
return null;
}
Expand Down

0 comments on commit 37449b1

Please sign in to comment.