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

Latest version of paper #2945

Closed
Raphy123 opened this issue Apr 24, 2020 · 27 comments
Closed

Latest version of paper #2945

Raphy123 opened this issue Apr 24, 2020 · 27 comments
Labels
completed The issue has been fully resolved and the change will be in the next Skript update.

Comments

@Raphy123
Copy link

Description

With the latest version of paper #210, 211, 212, 213, big bug and spam on console
https://pastebin.com/GeehQTEy

Errors / Screenshots

https://pastebin.com/GeehQTEy

Server Information

  • Server version/platform: 1.15.2
  • Skript version: 2.4.1

Additional Context

Thank you very much

@iOshawott
Copy link

I think it's important problem

@Raphy123
Copy link
Author

I really hope that a fix is ​​planned because I can no longer do the updates at all

@ShaneBeee
Copy link
Contributor

Try the alpha3 version.
Because I just updated paper on my server, and Skript loads fine

> ver skript
[03:53:36 INFO]: Skript version 2.5-alpha3
[03:53:36 INFO]: Customize Minecraft's mechanics with simple scripts written in plain English sentences.
[03:53:36 INFO]: Website: https://skriptlang.github.io/Skript
[03:53:36 INFO]: Authors: Njol, Mirreski, bensku, TheBentoBox, FranKusmiruk, ShaneBeee, Blueyescat, JRoy, Nicofisi, APickledWalrus, xXAndrew28Xx, Syst3ms, TheLimeGlass, Pikachu920, Sashie, Wealthyturtle, OfficialDonut and eyesniper2
> ver
[03:53:37 INFO]: This server is running Paper version git-Paper-214 (MC: 1.15.2) (Implementing API version 1.15.2-R0.1-SNAPSHOT)
[03:53:37 INFO]: Checking version, please wait...
[03:53:38 INFO]: Previous version: git-Paper-143 (MC: 1.15.2)
[03:53:38 INFO]: You are running the latest version
> 

@Raphy123
Copy link
Author

I have the same problem with the alpha. This is so strange. Before the update, all is ok, after, problem :/

@ShaneBeee
Copy link
Contributor

Try updating your java to OpenJDK.... this has solved a different issue regarding regex for other people, maybe that will work for this too.

https://adoptopenjdk.net

@iOshawott
Copy link

I have java OpenJDK and it still occurs. I can't update to alpha-3 since event-item doesn't work on 2.5 :(

@myavuzokumus
Copy link

Yeah, i have a same problem. And its problem of me.

@iOshawott
Copy link

I can give my logs if it's helpful, please help with it

@BenceX100
Copy link

It happens to me as well. When i updated my paper from ver205 to ver210 it stopped working.

@Raphy123
Copy link
Author

One of the solutions found is to double the time out in spigot ...

@iOshawott
Copy link

well, but doubling it is safe?

@Raphy123
Copy link
Author

Personally everything works again. Skript with its error takes longer to load but works. It takes 58 seconds and the time out is 60, which is why the server stops immediately. I don't think there are any risks

@iOshawott
Copy link

It's the time for loading the whole server or just skript? Skript loads for ~15 seconds and then errors occurring

@arts7493
Copy link

set watch dog and time out to high numbers

@BetaBurnt
Copy link

Completely missed this report when I checked to see if it had been reported yet so I'll close my other report (here if people want the full info I put in it: #2961 )
Also reported it on Papers GitHub, which has had some responses - PaperMC/Paper#3294

Issue is specifically Paper build 210 (technically 211 as 210 was a failed build) and newer, if you backdate to 209 it goes away without changing anything with Skript, your scripts or anything else on the server.

@bensku bensku added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation). labels May 6, 2020
@bensku
Copy link
Member

bensku commented May 6, 2020

We need to fix this, unfortunately I don't know when we have time to do that.

@BetaBurnt
Copy link

BetaBurnt commented May 6, 2020

For the time being on 1.15 we can at least backdate the Paper build to avoid it, but hopefully you guys have time/chance to fix it before 1.16 drops as all Paper 1.16 builds would have the issue :/

@noftaly
Copy link
Contributor

noftaly commented May 6, 2020

I don't know if it is worth saying, but with Paper-220 and 254, 1.15.2, Java 8 and Skript 2.4.1 I'm not having any issues at all, everything works fine. (on a local server on macOS, with only EssentialsX, FAWE and Skript)

[22:18:36 INFO]: This server is running Paper version git-Paper-254 (MC: 1.15.2) (Implementing API version 1.15.2-R0.1-SNAPSHOT)
[22:18:36 INFO]: Checking version, please wait...
[22:18:37 INFO]: Previous version: git-Paper-220 (MC: 1.15.2)
[22:18:37 INFO]: You are running the latest version
> ver skript
[22:18:39 INFO]: Skript version 2.4.1```

@BetaBurnt
Copy link

I don't know if it is worth saying, but with Paper-220 and 254, 1.15.2, Java 8 and Skript 2.4.1 I'm not having any issues at all, everything works fine. (on a local server on macOS, with only EssentialsX, FAWE and Skript)

[22:18:36 INFO]: This server is running Paper version git-Paper-254 (MC: 1.15.2) (Implementing API version 1.15.2-R0.1-SNAPSHOT)
[22:18:36 INFO]: Checking version, please wait...
[22:18:37 INFO]: Previous version: git-Paper-220 (MC: 1.15.2)
[22:18:37 INFO]: You are running the latest version
> ver skript
[22:18:39 INFO]: Skript version 2.4.1```

Issue is still present on Paper 1.15.2 build 254 on my test server (using only the plugins my scripts require to lighten the overall load, as per the shorter plugins list on the report I put up on PaperSpigot's GitHub) with no changes between that setup and testing on build 209 which works fine.
Is also present on build 220.

It doesn't happen with any/all Skript setups/amount of scripts, your setup is likely just not triggering it - however for those that it is happening to it doesn't happen if you backdate to Paper build 209 specifically.

@BetaBurnt
Copy link

For those with the issue update to Paper 1.15.2 build 256 or newer - Paper have applied their "band-aid" for the issue in this build so it stops happening, though they still say Skript needs to fix this properly their end.

@aikar
Copy link

aikar commented May 7, 2020

Well with our fix, Skript doesn't need to do anything.

I suspect Skript might actually be one of the very few plugins to justify this behavior to ensure the skripts load after ALL plugins are loaded without having to worry about depnendency hell.

I'm pretty happy with the solution, but if Spigot ever also starts the watchdog on the first tick, itll be back in same boat.

To explain the issue, Spigot doesn't engage watchdog on the very first tick, so the fact that Skript and other plugins delayed a task to run AFTER onEnable, caused it to run within the first tick.

Paper turned the watchdog on, as it's possible for your server to lock up in first tick still.....

So before, people with 3 minute long Skript loads had immunity from watchdog, and now don't.

On paper at least, now plugins are init BEFORE Done and before the watchdog starts, so startup time is correctly shown in "Done" too.

Skript can close this issue now though since nothing needs to be done.

though I hear there is a parallel mode thats off by default.... def should look at turning that on to speed up parsing, as the reporter taking around 3 minutes to load skripts is pretty insane.

@Pikachu920
Copy link
Member

Pikachu920 commented May 7, 2020

there's support for async parsing but not parallel parsing unfortunately. maybe one day, but for now the problem is njol coded skript to use static for everything everywhere and two sections parsing at once would break a billion things.

@SnowPyon
Copy link

SnowPyon commented May 7, 2020

We could still parallelize aliases parsing though, which does take a part of the initial load. That aside, we hope for skript-parser to be finished to make parsing step parallel at this point, since the original codebase was just not made to do this.

@aikar
Copy link

aikar commented May 8, 2020

When I looked at SkriptParser.java, it seems pretty safe to run that in parallel....
I could not see any signs of mutating any shared global state, and if there was even, a simple synchronized block around them would solve it.

That is the slow code, so ensuring a unique instance per parsed file (which I think is already the case) should be safe to parallelize that processing...

Sooo much regex and string parsing going on there, even if you synchronize every escape out of that file, you would still be net positive by significant margins.

I think it should be pretty doable to parallelize that even with current code.

@bensku
Copy link
Member

bensku commented May 8, 2020

Unfortunately, individual expressions sometimes refer to global state (e.g. ScriptLoader), and I think SkriptParser uses static methods relying on global state. It is probably possible to parallerize script parsing, but that would be a ton of work. Maybe more than replacing the entire parser with something faster.

Since it seems I mistakenly flagged this as a bug...

@bensku bensku removed the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label May 8, 2020
@bensku bensku removed the priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation). label May 8, 2020
@A248
Copy link

A248 commented May 15, 2020

Would it be possible to call SimpleExpression#init and other such methods which require global state sequentially while still parsing the strings & regex concurrently? It may be a lot of work, involving lots of synchronised/futures, but it would definitely improve startup times.

@A248
Copy link

A248 commented May 15, 2020

Also, this issue should probably be renamed.

@Pikachu920 Pikachu920 mentioned this issue May 19, 2020
@TPGamesNL TPGamesNL added PR available Issues which have a yet-to-be merged PR resolving it completed The issue has been fully resolved and the change will be in the next Skript update. and removed PR available Issues which have a yet-to-be merged PR resolving it labels May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed The issue has been fully resolved and the change will be in the next Skript update.
Projects
None yet
Development

No branches or pull requests