[bug] Fix async script reloading

[Olyno]

Description

This pull request includes a small fix for async event with EffScriptFile

---

Target Minecraft Versions: /
Requirements: /
Related Issues: /

[ShaneBeee]

Looks good to me 👍

[Pikachu920]

why is this change required? is there a related issue? or an explanation of what was wrong and how this fixed it?

[TPGamesNL]

loadScripts is called async if async script loading is enabled in the config, and the PreScriptLoadEvent is not an async event, so Bukkit throws an exception

[Pikachu920]

so it sounds like the real fix would be to make prescriptloadevent async when needed. if we run it sync all the time, it will not break some aspects of the event when parsing is async

[Olyno]

why is this change required? is there a related issue? or an explanation of what was wrong and how this fixed it?

Here is the context: I tried to execute the reload script file "my script" in an async Skript event, and like the bukkit PreScriptLoadEvent event is called async but not created for, it throws an error. This pull request fix this issue. I did a lot of tests and it seems to fix it correctly.
The issue has not been reported before this pull request.

[Pikachu920]

@Olyno can we just implement Event#isAsynchronous though

[Olyno]

Indeed I didn't know that, let me change that

[Olyno]

So after some tests, we can't only make an event async, we must execute it async. So it doesn't change a lot to make this event async or not, only the execution is enough and work fine.

[Pikachu920]

it does change things - if parsing is async and the event is called sync then anything listening to the event will be able to modify the script (which is 75% of the point of the event). what prevents you from making it async?

[FranKusmiruk]

I think there's a misunderstanding here, the event doesn't have to always be async. You've got to call super with ScriptLoader#isAsync like this:

public PreScriptLoadEvent(List<Config> scripts) {
  super(Scriptloader.isAsync());
  Validate.notNull(scripts);
  this.scripts = scripts;
}

this way you don't have to change how the event is called

[TPGamesNL]

There is one problem with ScriptLoader#isAsync for that: if you use the reload script effect in an async event, without having the option asynchronous script loading set tot true in the config (because that's what ScriptLoader#isAsync returns), the code from ScriptLoader#loadScripts will be called on the current thread, an async thread, while the event is created for sync usage.

[Pikachu920]

you can also just do super(Bukkit.isPrimaryThread())

[Olyno]

So after a lot of try, I still have an issue when Skript loads: https://0bin.net/paste/h3o9zvZx#ZYZlFmhHT1W3B69ALw3VnZ6G7Q95lRcEYHeuGLSgNCE

My first way was probably the best to fix that issue I think.

[Pikachu920]

what did your code look like when you got that exception?

[Olyno]

I simply created an async event, and execute it, like

on test:
	reload script "test"

[Pikachu920]

that would go away if you put super(Bukkit.isPrimaryThread()) in the constructor of the event

[Olyno]

It's exactly what I did. Anyway, it's not the super which will fix the issue. The super method only specify if an event is async or not, it doesn't fix its execution at all.

[Pikachu920]

we don't want to specify it's execution, we want it to be both async or sync depending on what parsing is. i just took a look at the spigot code and it definitely won't throw that exception if you use super(Bukkit.isPrimaryThread()):

  public Event(boolean isAsync) {
    this.async = isAsync;
  }

  public final boolean isAsynchronous() {
    return this.async;
  }

  
  public void callEvent(@NotNull Event event) {
    if (event.isAsynchronous()) {
      if (Thread.holdsLock(this))
        throw new IllegalStateException(String.valueOf(event.getEventName()) + " cannot be triggered asynchronously from inside synchronized code."); 
      if (this.server.isPrimaryThread())
        throw new IllegalStateException(String.valueOf(event.getEventName()) + " cannot be triggered asynchronously from primary server thread."); 
    } else if (!this.server.isPrimaryThread()) {
      throw new IllegalStateException(String.valueOf(event.getEventName()) + " cannot be triggered asynchronously from another thread.");
    } 
    fireEvent(event);
  }

though I can't find the exact line your exception points out - what version of paper are you running?

[TPGamesNL]

The Event constructor's argument is isAsync, not isSync: https://hub.spigotmc.org/javadocs/spigot/org/bukkit/event/Event.html#%3Cinit%3E(boolean). You need to invert Bukkit.isPrimaryThread()

[Pikachu920]

nice catch

[Pikachu920]

@Olyno can you try super(!Bukkit.isPrimaryThread())?

[Pikachu920]

@Olyno are you planning on making this change?

[Olyno]

@Olyno are you planning on making this change?

Yes I am, but it takes me some time due to personnal and profesionnal stuff, I do changes when I can

[Olyno]

So after did some tests, it seems to solve the problem @Pikachu920

[JRoy]

The following javadoc should be added to the class:

/**
 * This event has no guarantee of being on the main thread.
 * Please do not use bukkit api before checking {@link Bukkit#isPrimaryThread()}
 */

[Olyno]

Up ?