Online check in closeInventory

[bridgelol]

Adds a sanity check to CraftPlayer#closeInventory(InventoryCloseEvent.Reason) that ensures the player is online.

This fixes an edge case which can cause dupes in certain plugins.

Example
https://www.youtube.com/watch?v=OzaV7nUXKxQ -> 2:50

In this specific case the following happens;

Coinflip plugin plays an animation on the player, has a runnable that runs later on the current Player object to close its inventory when the animation is done.

Before this task runs the player disconnects and re-joins the server, opens a new gui (crate preview in this case) -> Coinflip plugin calls the closeInventory method on the old player object, crate plugin mistakes that for the current player instance closing their inventory, allowing the current player to take out items of the crate preview gui.

While yes, this is bad practice on the plugin's end, we should probably prevent plugin developers from messing this up.

[masmc05]

I think https://jd.papermc.io/paper/1.21.3/org/bukkit/OfflinePlayer.html#isConnected() will have more sense here than isOnline

[bridgelol]

I think https://jd.papermc.io/paper/1.21.3/org/bukkit/OfflinePlayer.html#isConnected() will have more sense here than isOnline

Agreed

[electronicboy]

I was going to comment and say that I'm somewhat precarious of this due to the risks, especially around existing plugin logic, doing things like closing an inventory before saving its contents in various places

moving this logic to isConnected() looks like it will cause this exception to occur inside of the PlayerQuitEvent, which makes this change 100% untenable

[bridgelol]

I was going to comment and say that I'm somewhat precarious of this due to the risks, especially around existing plugin logic, doing things like closing an inventory before saving its contents in various places

moving this logic to isConnected() looks like it will cause this exception to occur inside of the PlayerQuitEvent, which makes this change 100% untenable

Only problem is that isOnline gets the player based of its UUID which in this case wouldn't solve the underlying issue

[electronicboy]

Well, yea, both checks are flawed; I'm generally not sure that this is on us to solve, either; making API calls suddenly throw exceptions creates more issues than it solves, especially when we're only doing this to mitigate bad practices elsewhere and not an issue inside of the server/API

[bridgelol]

Well, yea, both checks are flawed; I'm generally not sure that this is on us to solve, either; making API calls suddenly throw exceptions creates more issues than it solves, especially when we're only doing this to mitigate bad practices elsewhere and not an issue inside of the server/API

Could argue that this method should never ever be called on an offline player and therefor the API should probably prevent you from doing that; just like how it prevents you from doing certain things asynchronously

[electronicboy]

I'm not saying that this shouldn't be prevented, I'm saying that I'm not fond of making a method throw an exception in a precarious place for other plugins which are going to be saving data, causing a whole host of duplication risks, to mitigate an issue in a plugin which isn't properly handling its own state

async catchers are also a bad argument, these are protecting against behavior which has been said to be bad for the better part of a decade and serve to prevent crashing the server

[Leguan16]

Well, yea, both checks are flawed; I'm generally not sure that this is on us to solve, either; making API calls suddenly throw exceptions creates more issues than it solves, especially when we're only doing this to mitigate bad practices elsewhere and not an issue inside of the server/API

Could argue that this method should never ever be called on an offline player and therefor the API should probably prevent you from doing that; just like how it prevents you from doing certain things asynchronously

Maybe adding JD to make devs aware that calling the method while the player is offline can cause issues makes more sense. And that they should check if the player is offline/online themself.

[electronicboy]

I'm also not seeing where Connection is set to null? I could get preventing the calls at some point after PlayerQuitEvent, but, not before it; I'm also generally not found of an exception, we should probably just do as little as possible there, not sure; Just not found of trading one dupe risk for another

[bridgelol]

We could also ensure the InventoryCloseEvent doesn't get called when it doesn't need to be (i.e this case)?

[bridgelol]

We could also ensure the InventoryCloseEvent doesn't get called when it doesn't need to be (i.e this case)?

Could do a contains check on PlayerList's players field to ensure the quit event has been processed