Skip to content

Ensure that unload event is called before unloading of functions regardless of order in script. #8250

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: dev/patch
Choose a base branch
from
Open

Ensure that unload event is called before unloading of functions regardless of order in script. #8250

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da519bd
Ensure that unload event is called before unloading of functions.
TFSMads Oct 22, 2025
cb40b35
Unload based on priority from structure
TFSMads Oct 22, 2025
bba3266
Remove unused import
TFSMads Oct 22, 2025
f024467
Remove debug message
TFSMads Oct 22, 2025
c2fd9f7
Merge branch 'dev/patch' into bug/unload
TFSMads Oct 30, 2025
9257163
Structures now unload based on priority across multiple scripts.
TFSMads Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 50 additions & 27 deletions src/main/java/ch/njol/skript/ScriptLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,18 +528,18 @@ private static CompletableFuture<ScriptInfo> loadScripts(List<Config> configs, O

// build sorted list
// this nest of pairs is terrible, but we need to keep the reference to the modifiable structures list
List<NonNullPair<LoadingScriptInfo, Structure>> pairs = scripts.stream()
List<LoadingStructure> loadingStructures = scripts.stream()
.flatMap(info -> { // Flatten each entry down to a stream of Script-Structure pairs
return info.structures.stream()
.map(structure -> new NonNullPair<>(info, structure));
.map(structure -> new LoadingStructure(info, structure));
})
.sorted(Comparator.comparing(pair -> pair.getSecond().getPriority()))
.sorted(Comparator.comparing(pair -> pair.structure().getPriority()))
.collect(Collectors.toCollection(ArrayList::new));

// pre-loading
pairs.removeIf(pair -> {
LoadingScriptInfo loadingInfo = pair.getFirst();
Structure structure = pair.getSecond();
loadingStructures.removeIf(loadingStructure -> {
LoadingScriptInfo loadingInfo = loadingStructure.loadingScriptInfo();
Structure structure = loadingStructure.structure();

parser.setActive(loadingInfo.script);
parser.setCurrentStructure(structure);
Expand All @@ -566,9 +566,9 @@ private static CompletableFuture<ScriptInfo> loadScripts(List<Config> configs, O
// Until these reworks happen, limiting main loading to asynchronous (not parallel) is the only choice we have.

// loading
pairs.removeIf(pair -> {
LoadingScriptInfo loadingInfo = pair.getFirst();
Structure structure = pair.getSecond();
loadingStructures.removeIf(loadingStructure -> {
LoadingScriptInfo loadingInfo = loadingStructure.loadingScriptInfo();
Structure structure = loadingStructure.structure();

parser.setActive(loadingInfo.script);
parser.setCurrentStructure(structure);
Expand All @@ -590,9 +590,9 @@ private static CompletableFuture<ScriptInfo> loadScripts(List<Config> configs, O
parser.setInactive();

// post-loading
pairs.removeIf(pair -> {
LoadingScriptInfo loadingInfo = pair.getFirst();
Structure structure = pair.getSecond();
loadingStructures.removeIf(loadingStructure -> {
LoadingScriptInfo loadingInfo = loadingStructure.loadingScriptInfo();
Structure structure = loadingStructure.structure();

parser.setActive(loadingInfo.script);
parser.setCurrentStructure(structure);
Expand Down Expand Up @@ -657,6 +657,8 @@ public LoadingScriptInfo(Script script, List<Structure> structures, Map<Structur

}

public record LoadingStructure (LoadingScriptInfo loadingScriptInfo, Structure structure) {}
Copy link
Member

@APickledWalrus APickledWalrus Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd be fine to define this within the method itself since it isn't used elsewhere


/**
* Creates a script and loads the provided config into it.
* @param config The config to load into a script.
Expand Down Expand Up @@ -854,6 +856,8 @@ private static Config loadStructure(InputStream source, String name) {
* Script Unloading Methods
*/

public record UnloadingStructure (Script script, Structure structure) {}
Copy link
Member

@APickledWalrus APickledWalrus Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This can be defined in the method.


/**
* Unloads all scripts present in the provided collection.
* @param scripts The scripts to unload.
Expand All @@ -870,35 +874,54 @@ public static ScriptInfo unloadScripts(Set<Script> scripts) {
}

ParserInstance parser = getParser();
Comparator<UnloadingStructure> unloadComparator = Comparator.comparing(unloadingStructure -> unloadingStructure.structure().getPriority());
unloadComparator = unloadComparator.reversed();
Comment on lines +877 to +878
Copy link
Member

@APickledWalrus APickledWalrus Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only used once in the stream operations, I don't think it needs to be separately declared anymore.


// initial unload stage
for (Script script : scripts) {
parser.setActive(script);

// trigger unload event before beginning

List<UnloadingStructure> unloadingStructures = scripts.stream()
.flatMap(script -> { // Flatten each entry down to a stream of Script-Structure pairs
return script.getStructures().stream()
.map(structure -> new UnloadingStructure(script, structure));
})
.sorted(unloadComparator)
.collect(Collectors.toCollection(ArrayList::new));

// trigger unload event before unloading scripts
for (Script script : scripts) {
eventRegistry().events(ScriptUnloadEvent.class)
.forEach(event -> event.onUnload(parser, script));
.forEach(event -> event.onUnload(parser, script));
script.eventRegistry().events(ScriptUnloadEvent.class)
.forEach(event -> event.onUnload(parser, script));
.forEach(event -> event.onUnload(parser, script));
}

for (Structure structure : script.getStructures())
structure.unload();
// initial unload stage
for (UnloadingStructure unloadingStructure : unloadingStructures) {
Script script = unloadingStructure.script();
Structure structure = unloadingStructure.structure();

parser.setActive(script);
structure.unload();
}

parser.setInactive();

// finish unloading + data collection
// finish unloading of structures + data collection
ScriptInfo info = new ScriptInfo();
for (Script script : scripts) {
List<Structure> structures = script.getStructures();
for (UnloadingStructure unloadingStructure : unloadingStructures) {
Script script = unloadingStructure.script();
Structure structure = unloadingStructure.structure();

info.files++;
info.structures += structures.size();
info.structures++;

parser.setActive(script);
for (Structure structure : structures)
structure.postUnload();
structure.postUnload();
parser.setInactive();
Copy link
Member

@APickledWalrus APickledWalrus Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be moved right after the loop so that it is only called once.

}

// finish unloading of scripts + data collection
for (Script script : scripts) {
info.files++;

script.clearData();
script.invalidate();
Expand Down