search

skyrising / skyblock

Minecraft mod for empty world generation and new ways to get certain items
MIT License
46 stars 13 forks source link

NPE when syncing rule in fabric-carpet in the client #23

Closed altrisi closed 3 years ago

altrisi commented 3 years ago

In fabric-carpet, rules are synced with clients. That sync behaviour happens when the player joins and when someone changes a rule in-game. ~The syncing behaviour is currently broken for extension's custom SettingsManagers (see gnembon/fabric-carpet#546), which is why this issue hasn't really occurred (yet).~ The changes have been pushed to carpet.

However, I'm currently fixing that issue, and I've encountered this problem while testing (that took me hours to realize it wasn't an issue in my code since minecraft doesn't print stacktraces from network by itself).

This NPE kicks/prevents players from joining servers with skyblock when having it in their client too.

java.lang.NullPointerException
    at skyblock.SkyBlockSettings$BetterPotionListener.validate(SkyBlockSettings.java:25)
    at skyblock.SkyBlockSettings$BetterPotionListener.validate(SkyBlockSettings.java:21)
    at carpet.settings.ParsedRule.set(ParsedRule.java:162)
    at carpet.settings.ParsedRule.set(ParsedRule.java:134)
...

Reason: When syncing, Carpet (client) will pass null as the ServerCommandSource to the rule. The rule will then call its set method to get parsed, since in the network it gets sent as a string, and will call its validators. The betterPotions rule uses that source when being validated, and will assume that it isn't null, when in this case it will be.

https://github.com/skyrising/skyblock/blob/501c19706dd338143e6e34638e4978e9ca2b698a/src/main/java/skyblock/SkyBlockSettings.java#L23-L28

Solutions: Check whether the source is null before using it in betterPotions or get it from a different place (since it would be in the client and you are searching for a MinecraftServer, probably the first one).

altrisi commented 3 years ago

Would make a PR, but there is no license specified.

altrisi commented 3 years ago

The changes have been pushed to fabric-carpet. This will be an issue in the next release if not fixed, without giving any useful feedback to the user (except if extra checks are added to fabric-carpet, which I don't think will happen).

And it's as easy to fix as wrapping the above code into if (source != null) { ... }

DeadlyMC commented 3 years ago

I just made a commit, so i think this issue can be closed now. Im gonna wait for your confirmation @altrisi and then make a release

altrisi commented 3 years ago

Seems to be fixed in last commit, thanks!