bergerhealer / BKCommonLib

An extensive library used in bergerhealer's plugins
Other
181 stars 45 forks source link

BKCommonLib modifies world entities list in an unsafe way causing issues for other plugins #57

Closed basicmark closed 5 years ago

basicmark commented 5 years ago
BkCommonLib version:

BKCommonLib-1.13.2-v1-SNAPSHOT.ja

Spigot version:

git-Spigot-f56e2e7-ad6070d (MC: 1.13.2) (Implementing API version 1.13.2-R0.1-SNAPSHOT)


Problem or bug:

BKCommonLib interfers with the World entity lists causing issues for other plugins which use said lists, specifcally itemframes this means that although the server returns true to world.isChunkLoaded(x,z) the world entity lists are not (fully) updated with the entities within the chunk and this thus leads to an inconsistently. This is espically true when check adjacent chunks during chunk loading. The issue was oringally seen in a custom plugin but a simplified test plugin has been created.

This issue was orginally thought to be a spigot bug so https://hub.spigotmc.org/jira/browse/SPIGOT-4603 was raised. Please review the comments there for further details and the test plugin.

Expected behaviour:

If the world.isChunkLoaded(x,z) returns true all the entities within the chunk have been added to the worlds lists.

Steps to reproduce:

1) Create a test server with at least BKCommonLib and ChunkTest. The full list of plugins from my test server are:

ChunkTest-1.1.jar EssentialsX-2.15.0.52.jar BKCommonLib-1.13.2-v1-SNAPSHOT.jar EssentialsXSpawn-2.15.0.52.jar PermissionsEx-1.23.4.jar

2) Move away from spawn into so the server will have to unload and load chunks when teleport.

3) Place a large number of itemframes on blocks which are in a different chunk (if this is not clear the picture in https://www.spigotmc.org/threads/chunkloadevent-async-on-paper.356923/#post-3292361 might help).

4) Telelport away and wait for the chunks to unload (I used /spawn for this)

5) Telelport back (I used /back for this).

6) Review the server log after telelporting back and see that ChunkTest has reported that an entity is not in the world, the message looks something like:

[23:53:48] [Server thread/INFO]: Chunk entity in world false(CraftItemFrame{item=ItemStack{AIR x 1}, rotation=NONE})

Without BKCommonLib this debug always returns true showing there is no inconsistency.

bergerkiller commented 5 years ago

Hmmn BKCommonLib only writes/alters the entity list when traincarts is installed, which isn't the case here. There is no such entity list manipulation for handling the item frame / map display logic. However, there might be some asynchronous access going on because of how the packet listening works.

Ignore this, does not fix it< If you got the time for it, can you test this custom build out? REMOVED I've disabled packet handling of the OUT_ENTITY_METADATA packet, which is/was used for translating map items inside item frames.

I'll have to try and put time aside to look into this myself

I'd also try installing ProtocolLib to see if that has any effect

bergerkiller commented 5 years ago

I could reproduce this quickly, so that's at least good news. I don't think the issue will be very complicated to solve.

bergerkiller commented 5 years ago

Problematic code is right here: https://github.com/bergerhealer/BKCommonLib/blob/3e54cceb29c9cf1f5c1f91d758930e96585ffab9/src/main/java/com/bergerkiller/bukkit/common/internal/CommonMapController.java#L682

Don't mind the TODO, it's not like a time traveler is trying to warn me...

bergerkiller commented 5 years ago

I've confirmed myself that this issue is resolved with this version, can you check it as well? https://ci.mg-dev.eu/job/BKCommonLib/509/

basicmark commented 5 years ago

I can confirm on my test server 509 does resolve the issue.

Thanks for the support :)