FastAsyncWorldEdit

FastAsyncWorldEdit

Bukkit Plugins
152k Downloads

Inventory modifying async

DeeCaaD opened this issue ยท 4 comments

commented

Server Implementation

Paper

Server Version

1.19.2

Describe the bug

FAWE is doing inventory modifying async (remove and add).

Our equip listener detected it at at com.sk89q.worldedit.bukkit.BukkitPlayer.giveItem(BukkitPlayer.java:169) ~[FastAsyncWorldEdit-Bukkit-2.4.5-SNAPSHOT-278.jar:?]

After checking the code there, it seems like you go sync in that method, but that whole method should be done sync, not just the event call.
https://github.com/IntellectualSites/FastAsyncWorldEdit/blob/main/worldedit-bukkit/src/main/java/com/sk89q/worldedit/bukkit/BukkitPlayer.java#L172

To Reproduce

  1. Do for example //wand command

Expected behaviour

No inventory control is done async.

Screenshots / Videos

No response

Error log (if applicable)

No response

Fawe Debugpaste

https://athion.net/ISPaster/paste/view/da4e054392934de09e2d04a06112e920

Fawe Version

2.4.5

Checklist

Anything else?

Inventory control should not be done async. I think you understand when I say it isn't safe

commented

Please provide a minimum viable product, that outlines an issue with the current code in a real world scenario.

commented

I think you mean example issues this can cause? For example:

  • delete or duplicate items (rarely)
  • interfere with any plugin that modifies the inventory
  • breaks our plugin's equip listener
    • we only give warning about plugin which is doing async inventory modification, and tell to ask its dev to fix it

In order to fix this, line number 172 has to be at the start of that method.

I can see that you think that something like this can nearly never happen, but that doesn't mean it shouldn't be fixed. Having those lines async doesn't actually give any performance boost, but only issues if something in sync tries to modify inventory at same time.

commented

breaks our plugin's equip listener

That's why I asked for a minimum viable product, to analyze the case presented in a practical state, rather than going over it in theory only.

I can see that you think that something like this can nearly never happen

I have no idea why you would assume what I'd think, given it's the exact opposite of my standing.

commented

I have no idea why you would assume what I'd think, given it's the exact opposite of my standing.

Ah okay sorry for that. Other dev in WM project already mentioned about this in your discord some time ago, but he was basically told that it doesn't matter since 1000s of servers already use it, thats why I had such assumption.

But yeah this is our EquipListener. We could fix this and move that back to sync in our own code, but we'd rather encourage other devs to not do inventory modification in async. The logging is at 250 when doing async modifications to inventory. I also apologize if there is some rude comments, CJ was kinda annoyed when he found out about those (he coded the EquipListener stuff). FAWE wasn't the only one doing this.