Fabric API

Fabric API

106M Downloads

`isIn` on item sometimes causes NPE in TagDelegate

LaylBongers opened this issue ยท 12 comments

commented

This is a really mysterious bug.

I've got a mixin on Block#calcBlockBreakingDelta that checks if the item the player is holding is in a specific tag, using isIn. When this happens, sometimes an NPE is thrown from TagDelegate#contains.

However, nothing seems to be null.

idea64_2019-11-03_02-46-25

This happens roughly 1 in every 5 runs randomly, and only once. This is the deepest in the stack trace reported.

commented

The delegating mechanism behind TagRegistry allows mods to hold tags in static final fields similar to vanilla's BlockTags and similar classes. This is potentially faster than querying the tag from scratch while automatically adjusting to updates.

Fabric's current implementation is definitely bugged since there is nothing prohibiting such an use that I am aware of. Tags are supposedly universally usable.

commented

Just curious, is the code called from the client thread?

commented

It's called from both client and server

commented

isIn can only be called safely from the server thread, so you need an alternative way (getting the client world's tag manager instead)

commented

These crashes happen on both the server side and the client side. What's unsafe about calling isIn? It really just does a dictionary lookup as far as I can tell.

commented

the problem is the dictionary you are looking up is bound to the logical server and the server thread, while some of your calls come from the client thread (client player interaction manager). It is already good enough that your game did not end up with something like cme.

commented

Ah, so this is a race condition? That explains the weird only occasional crashes. However, I haven't managed to find out how to use the client world's tag manager yet. How do I use that to compare a block/item to a tag?

commented

So this turns out to be a race condition with the lazy initialization when it's called from both the client and the server at the same time. To fix this a good approach is to retrieve the tag by Identifier from the World's tag manager, which will be unique to the side it's being accessed from.

    private static boolean checkBlockContains(World world, Identifier tagId, Block block) {
        // Performs a tag contains check independent of side, will crash if used on both client and server otherwise.
        Tag<Block> tag = world.getTagManager().blocks().get(tagId);
        
        if (tag != null) {
            return tag.contains(block);
        } else {
            return false;
        }
    }

World can be accessed by casting BlockView into it.

Beyond that, does this mean it's a Fabric bug? The bug happens in Fabric code purely because of how Fabric has implemented something. But, from what I understand classes with static fields with tags like Fabric's tool tags are intended to be used only on the server anyways?

commented

Since that in 1.16 minecraft check when every block is broken for :

this.isIn(BlockTags.GUARDED_BY_PIGLINS)

And while playing minecraft in .16.2 with fabric Loader 0.9.2+build.206 and fabric-api 0.19.0+build.398-1.16, I never had a single crash or error for TagDelegate#contains. I assume this mean it was fixed.

EDIT : Sorry for the wrong test :(

commented

@uiytt You misunderstood here. BlockTags are vanilla tags using vanilla constructs. This problem is with fabric api-provided identified tags that doesn't always update when vanilla tag managers reload from disc.

commented

Does this issue still occur after #423

commented

Issue seems to have been fixed.