Regression: PlayerDeathEvent custom drops are mutated into AIR by the server after the event is handled

[0dinD]

Expected behavior

PlayerDeathEvent custom drops are not mutated in any way by the server after the event is handled.

Observed/Actual behavior

PlayerDeathEvent custom drops are mutated into AIR by the server after the event is handled.

Steps/models to reproduce

Given the following (somewhat contrived) example code:

package org.example.plugin;

import org.bukkit.Material;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.entity.PlayerDeathEvent;
import org.bukkit.inventory.ItemStack;
import org.bukkit.plugin.java.JavaPlugin;

public class ExamplePlugin extends JavaPlugin implements Listener {

	@Override
	public void onEnable() {
		getServer().getPluginManager().registerEvents(this, this);
	}

	@EventHandler
	public void onPlayerDeath(PlayerDeathEvent e) {
		final ItemStack itemStack = new ItemStack(Material.DIAMOND);
		// Any custom drops added are affected by this issue
		e.getDrops().add(itemStack);

		// This one prints the diamond, it's fine.
		getLogger().info(">>> Item stack before PlayerDeathEvent: " + itemStack);
		getServer().getScheduler().runTask(this, () -> {
			// This one prints AIR, where did the diamond go?
			// If you attach a debugger you can see that the underlying NMS item is still
			// a diamond, but the item stack count is set to 0, which is why the item is treated as AIR.
			getLogger().info(">>> Item stack after PlayerDeathEvent: " + itemStack);
		});
	}

}

You should be able to observe the following output when a player dies (e.g. using /kill):

[ExamplePlugin] >>> Item stack before PlayerDeathEvent: ItemStack{DIAMOND x 1}
[ExamplePlugin] >>> Item stack after PlayerDeathEvent: ItemStack{AIR x 0}

As you can see, the item stack added as a custom drop was mutated into AIR by the server. Note that this issue only affects custom drops added, not any of the original drops that the player had in their inventory.

Plugin and Datapack List

[03:14:44 INFO]: Server Plugins (1):
[03:14:44 INFO]: Bukkit Plugins:
[03:14:44 INFO]:  - ExamplePlugin

[03:15:38 INFO]: There are 3 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)], [paper (built-in)]
[03:15:38 INFO]: There are no more data packs available

Paper version

[03:16:13 INFO]: Checking version, please wait...
[03:16:13 INFO]: This server is running Paper version 1.21.1-128-master@d348cb8 (2024-10-21T16:23:24Z) (Implementing API version 1.21.1-R0.1-SNAPSHOT)
You are running the latest version

Other

After some testing, I can conclude that this seems to be a regression introduced in build 79 for Minecraft 1.21.1, i.e. commit 0a53f1d. On earlier builds, the drops are not mutated into air and so both of the log statements in the example code print a diamond, as expected.

I didn't debug far enough to tell what code path is setting the item stack count to 0, but to me it seems like the regression introduced in build 79 is that the custom drop item stacks are no longer copied, so if the server mutates them then that will now affect the API user. I'm not entirely sure if the API user should guard against this by cloning the item stack or if this is a bug that should be fixed in Paper, but I'm leaning toward the latter. In which case, please see my PR #11521 for a possible patch to fix this regression.

Let me know if this seems correct or if I have perhaps misunderstood something.

[electronicboy]

While this might be a behavioral change that should be addressed, I would generally note that cloning items before you toss them into other places is a good idea, as there are 0 promises in the long term of how these items are handled

[0dinD]

Yeah sounds fair, that's why I was not sure if this should be considered a Paper bug or API user error.

there are 0 promises in the long term of how these items are handled

So in the general case, whenever you pass an ItemStack to the API that does something with the ItemStack "later", such as adding ItemStacks to PlayerDeathEvent#getDrops(), you should expect that the server will "clobber" it, i.e. you, the caller, needs to save (clone) it if you don't want it to change? Even if the thing you're passing it to seems "harmless"? Like, in the PlayerDeathEvent drops case, my naive expectation would be that the server just has to read the ItemStack in order to drop it, not write to it (i.e. set the count to 0).

I'm not that familiar with the server internals but I understand if "0 promises" has to be the case for one reason or another. Still, I can't help but feel that it's a bit unintuitive, and a potential pitfall for newcomers to the API. Do you have any long-term plans to address this, i.e. in terms of documentation or improved API (some kind of immutable ItemStack?). I'm just a bit curious, that's all.

Edit: Hmm, the more I think about it, the more I get this feeling that yeah, why should you expect that the server does not modify ItemStacks you pass to it? It kind of makes sense that if ItemStacks are mutable and "live" in the server, the server will probably end up mutating them at some point unless copies are made at every point. So I think I kind of get it. Still feels like a pitfall though.

---

I will also add that I'm maintaining an old plugin that someone else wrote, so I have no idea just how many potential pitfalls are hiding in the codebase if "0 promises" is the case. I guess up until now I've just been relying on the API to copy/clone things for me, and then a few days ago I noticed this particular bug with PlayerDeathEvent (which took me a while to get to the bottom of, I must say). I guess it's just "tough luck" for me, having to maintain code that someone else wrote. I don't suppose you have any ideas on how I could more easily discover more of these cases (if PlayerDeathEvent drops is not the only one)?

[electronicboy]

the server just has to read the ItemStack in order to drop it

There are 0 promises that the server will blindly clone an ItemStack you put into the world for you, a lot of the old behavior here was somewhat a mixture of happenchance or the fact that ItemStacks often API driven which is no longer the case; The behavioral change should probably be fixed, but, long run, you should be cloning any stack you don't want to be modified outside of your control when you're putting it into the world

The only thing we could ever do here is start blindly cloning ItemStack instances all around the API, but there is no real great consistency in the internals or the API here in terms of how this sort of thing is handled, and there is a lot of reliance on the fact that ItemStacks are not blindly cloned all over the place to prevent these issues

[0dinD]

Yeah this is starting to click in my head now, thanks for the input. I think part of my confusion comes from these API inconsistencies and "hidden" happenchance behaviors around ItemStack. Even though it seems obvious in hindsight, I hadn't really considered that a consequence of adding the ItemStack to the drops is that it becomes "live" in the world. And I feel like it's very common for the API to hide this fact because it in fact does copy the ItemStack in many cases.

I also think it's interesting to compare ItemStack to Block and Entity. For the latter two, I think it is quite intuitive to understand that they are "live" and mutable objects in the world, so you should expect them to change. And both also have immutable snapshot variants (BlockState and EntitySnapshot) that you can use if you want just that, an immutable snapshot (which is also an intuitive concept).

But ItemStack is this weird middle ground where it can be "live" in the world, but it can also be "disconnected" from the world if you just create it in the API and don't add it to the world. Although since it always can be added to the world it seems like in the general case you should assume that it is live in the world. Also, in contrast to Block/BlockState and Entity/EntitySnapshot, ItemStack has no snapshot variant (to my knowledge), so it is always mutable.

I can make ItemStack clones, and will probably do so in more places now that I've become aware of these issues. But I feel like clones only protect me "short-term" and not "long-term" in the sense that if I accidentally leak a cloned ItemStack somewhere, it might become live and change again. Having an immutable ItemStack snapshot would allow me to protect against such mistakes.

Given all the above, two questions come to mind, if you don't mind answering:

1. Are there any (long-term) plans to add API for ItemStack snapshots? Just curious, I understand that it might take quite a bit of work.

2. The Javadocs for Block have a warning that reads:

   This is a live object, and only one Block may exist for any given location in a world. The state of the block may change concurrently to your own handling of it; use block.getState() to get a snapshot state of a block which will not be modified.

   What are your thoughts on adding a similar warning to ItemStack, if you agree that in the general case, one should assume that an ItemStack is live in the world?

---

Edit: After thinking some more I realize that BlockState is not immutable, so I guess in a way it's more similar to cloning an ItemStack than it is to EntitySnapshot. But even though a BlockState is mutable, the BlockState object itself cannot become "live" in the world, so I still feel like it protects me from having it accidentally becoming live if I leak it to the server somehere. The same cannot be said if I clone an ItemStack.

[FireInstall]

BlockState can be "live" in the world. It isn't always a snapshot, its one just per default.
You can obtain a "live" state via Block#getState(false); be careful with it, as it can easy lead to unexpected things to happen.

[0dinD]

That is a very good point, I had seen that method overload but somehow didn't quite realize the implications (the BlockState becoming live) until you just pointed it out. Arguably that overload can be very useful, but it's also another potential pitfall, similar to ItemStacks. I would also once again argue that maybe the Javadocs could be improved here, because the BlockState Javadoc does not make this obvious at all. It says:

Represents a captured state of a block, which will not change automatically.

I feel like that's no longer true in the general case now that Paper has Block#getState(false) which will get you a "live" BlockState that can change "automatically", as far as I see it. The Javadoc was previously true, and I think still is, on Spigot (which doesn't have this overload on Block#getState).

I think another reason that may lead people to believe that BlockState is a snapshot is the following Javadoc on Block:

The state of the block may change concurrently to your own handling of it; use block.getState() to get a snapshot state of a block which will not be modified.

Technically that's not wrong though, because it refers to the original variant of Block#getState which actually does capture a snapshot. But that kind of makes it easy to fall into the trap of concluding that BlockState must itself always be a snapshot, which it now is not.

---

Anyways, I feel like adding some Javadoc warnings to ItemStack and BlockState (which clarify that they may be live in the world) would be a good thing. Something like the warning for Block:

This is a live object, and only one Block may exist for any given location in a world. The state of the block may change concurrently to your own handling of it

---

Regarding point (1) in my previous comment, I ended up creating an ItemStack wrapper class that is basically a snapshot. It works by initially making a copy of the stack which is to be snapshotted, and then "protecting" that copy so that it's hard to make it accidentally become live. I won't go into detail because it's probably specific to my use case and I still want to evaluate and iterate on the concept. But I'm basically just saying that I found a workable solution for that part, so I don't really need an ItemStack snapshot in the API (though I am still curious, of course).