IslandCache#islandsByUUID population

[MeFisto94]

Expected behavior

IslandsManager#getIsland(World, UUID) to return the Island (the first one) the player belongs to.

Observed/Actual behavior

The method returns null, iterating over IslandsManager#getIsland() and filtering gets me there.

Steps/models to reproduce

Call IslandManager.getIsland(world, playser.getUniqueId())

BentoBox version

[02:48:48 INFO]: Running PAPER 1.20.4. [02:48:48 INFO]: BentoBox version: 2.0.0-SNAPSHOT-b2521 [02:48:48 INFO]: Database: JSON [02:48:48 INFO]: Loaded Game Worlds: [02:48:48 INFO]: islands (AcidIsland): Overworld, Nether, The End [02:48:48 INFO]: Loaded Addons: [02:48:48 INFO]: AcidIsland 1.18.1-SNAPSHOT-b1008 (ENABLED) [02:48:48 INFO]: Challenges 1.3.0-SNAPSHOT-b539 (ENABLED) [02:48:48 INFO]: ControlPanel 1.13.1-SNAPSHOT-b61 (ENABLED) [02:48:48 INFO]: Level 2.12.0-SNAPSHOT-b555 (ENABLED)

Plugin list

No response

Other

If I've interpreted the code around islandsByUUID correctly, it only contains entries when the player has been added to the island in that current runtime. Especially the computerIfAbsent statements should probably pre-populate the HashSets in a reasonable way, if the server is restarted.

[MeFisto94]

I think this also has side effects for built in commands not working for us (/island go), e.g. in private CompletableFuture<Location> getAsyncSafeHomeLocation(@NonNull World world, @NonNull User user, String homeName) in IslandsManager.

I think getIslandLocation is null here and we just fail. And also the user does not get any error feedback in this case.

[tastybento]

Can you give me some example code of where this returns null? E.g. a minimal addon or plugin that yields this result beause I cannot replicate. When the islands are loaded when the server starts, the islandsByUUID map is populated. If you are getting a null, then in theory the player doesn't have an island in the world specified.

The code in IslandCache for getting is basically this:

        return islandsByUUID.computeIfAbsent(uuid, k -> new HashSet<>()).stream().filter(i -> w.equals(i.getWorld()))
                .collect(Collectors.toSet());

[MeFisto94]

So I can't give you exact replication information, we don't restart the server every night, but something in that mix certainly causes said cache to be imperfect, leading to issues such as /island go to fail (it will tell you that you're getting ported and then nothing happens) or the following exception, that I couldn't completely trace back, maybe it's something else, but:

java.lang.NullPointerException: Cannot invoke "world.bentobox.bentobox.api.panels.TabbedPanel.getIsland()" because "this.parent" is null
        at world.bentobox.bentobox.panels.settings.SettingsTab.<init>(SettingsTab.java:57) ~[BentoBox-2.0.0-SNAPSHOT-b2526.jar:?]
        at world.bentobox.bentobox.api.commands.island.IslandSettingsCommand.execute(IslandSettingsCommand.java:51) ~[BentoBox-2.0.0-SNAPSHOT-b2526.jar:?]
        at world.bentobox.bentobox.api.commands.CompositeCommand.call(CompositeCommand.java:292) ~[BentoBox-2.0.0-SNAPSHOT-b2526.jar:?]
        at world.bentobox.bentobox.api.commands.CompositeCommand.execute(CompositeCommand.java:260) ~[BentoBox-2.0.0-SNAPSHOT-b2526.jar:?]

Anyway, my minimal plugin is indeed just a command that calls IslandManager#getIsland to return an error message for players that don't have an island, but this was always false for everyone after a server restart ( I guess?).

Specifically your code snippet:

islandsByUUID.computeIfAbsent(uuid, k -> new HashSet<>())

This is kinda equivalent to

if (!islandsByUUID.containsKey(uuid))) {
  return new HashSet<>();
}

So it doesn't really populate the cache, it creates an empty cache. Other methods, such as "hasIsland" also work slightly different, if there's no cache present, they expect it to be false, in which case, the usage of computeIfAbsent is a bit contradictionary.

Cases in which the cache is actually filled, is when setOwner, addPlayer or addIsland are called. Now I don't know if those are supposed to happen during load, but the constructor itself doesn't do any loading. Oh, I'm now seeing that IslandsManager#load is supposed to call addIsland on the cache, so I can't exactly tell you what I am missing here.

[tastybento]

Yeah, load() loads all the islands on startup, so every one should be in there.

I recently fixed a bug with teleporting home that sounds like the sort of issue you saw (https://github.com/BentoBoxWorld/BentoBox/pull/2267). Hopefully that'll help if you were seeing home teleport weirdness.

[MeFisto94]

So as it turns out, the exception that happens with the settings menu is now consistent for every user and also not solved with a reboot, it could be a regression from going to the most recent version due to the chest boat. Can you replicate that specifically?

As for the teleport/islandcache, I can't give you more insight as of yet. Maybe I could add some periodic debug logging to check the cache state on startup and at a later state, too. Since it happens for some players it feels like being related to some timing related thing, such as them joining/leaving at some point, doing some things, in relation to the server startpoint.

[tastybento]

Can you share logs? I don't see any errors so i'm not sure what you are seeing.

[MeFisto94]

java.lang.NullPointerException: Cannot invoke "world.bentobox.bentobox.api.panels.TabbedPanel.getIsland()" because "this.parent" is null
        at world.bentobox.bentobox.panels.settings.SettingsTab.<init>(SettingsTab.java:57) ~[BentoBox-2.0.0-SNAPSHOT-b2526.jar:?]
This has been solved with https://github.com/BentoBoxWorld/BentoBox/commit/edd7bcfbd2e3ac2d54d769328074317f735aa479 I guess, so I'll update first. 

I'll also report back how the caching now works, but to give you the exact code I am using again (but I guess you'd still lack the side effects I have):

        return BentoBox.getInstance().getIslands().getIslands().stream().filter(island -> island.getMemberSet().contains(uuid)).findAny();
        //return Optional.ofNullable(BentoBox.getInstance().getIslands().getIsland(plugin.getIslandWorld(), uuid));

Basically, I want to find the island of uuid if there is any, and the commented out version is the code i used previously (pre-2.0) and the above is the workaround I used.