XP drain on tank causes server crash (1.12.2-9.7.4.375)

[CJenkins93]

Please use the search functionality before reporting an issue. Also take a look at the closed issues!

Issue description:

Basic fluid tank crashes server upon standing on an open blocks 'xp drain' placed on top.

Steps to reproduce:

1. Start a multiplayer server and join. (Using Enigmatica 2 Expert)
2. Place Basic tank and xp drain on top of it after getting some experience.
3. Stand on the drain and viola, server crash.

Version (make sure you are on the latest version before reporting):

Forge: 1.12.2-14.23.5.2836-universal Mekanism: 1.12.2-9.7.4.375 Mekanism Generators: 1.12.2-9.7.4.375

If a (crash)log is relevant for this issue, link it here: (It's almost always relevant)

https://pastebin.com/6Vdt81DF

[thiakil]

That's the XP Drain's issue - they're giving us an invalid FluidStack, seems it's missing a Fluid name or they're giving a Fluid that's not the chosen one from the registry

[boq]

Hi, OpenMods dev here. This is probably caused by some other mod making stupid things with fluid registry, but something caught my attention: how could one even create FluidStack that references non-chosen fluid? FluidStacks points to delegate, which is supposed to always resolve (by name) to primary ones. AFAIK the main idea is to create FluidStack with your instance on Fluid and let Forge resolve it.

[pupnewfster]

I agree it seems to be caused by some other mod doing stupid things with the fluid registry, though I am not sure there is a good way for us to handle it given what we are calling that is causing the crash to happen.

Based on the stack trace the code that is causing a crash (some parts are stripped out to better portray where the crash is happening)

void addFluidStack(FluidStack stack) {
    if (stack != null) {
        stack.writeToNBT(new NBTTagCompound()); //This call causes the null pointer
    }
}

I believe this is our call that "causes" the crash. The line in FluidStack#writeToNBT that the crash is happening in is:

nbt.setString("FluidName", FluidRegistry.getFluidName(getFluid()));
//And then the getFluidName which is what returns null causing the null pointer
public static String getFluidName(Fluid fluid) {
    return fluids.inverse().get(fluid);
}

My best idea why this may cause a crash (though this is mainly just speculation):

1. Some mod comes along and registers liquid xp as "xpjuice"
2. You then come along and try to register your liquid xp as "xpjuice" as well (this is fine in fact good that they should be able to recognize each other as the same one iirc how the fluid dictionary works). Though because they already have the entry in the map pointing to their Fluid object, it continues to point to it.
3. From what I can tell you reference it directly, but now you use your Fluid object directly instead of asking forge to use the Fluid object forge believes is "xpjuice" to create a FluidStack (this works fine as the Fluid object is not null).
4. When forge looks at the inverse of the BiMap, your liquid xp is not in it as it would have overridden the one that is already registered.
5. A null pointer is thrown as it can't find it based on your fluid.

A relatively quick way we probably could test if this is indeed the case is that after you register your liquid xp you then store in your cached Fluid object FluidRegistery.getFluid("xpjuice") instead of your own object. If it stops crashing then my theory is probably right about what is happening.

If not I am at a loss as calling FluidStack#writeToNBT on a non null fluidstack should not be causing a crash.

[boq]

This analysis is not taking into account delegates: when fluid stack is created, Forge lookups delegate for given fluid (which is assigned on registration). When proper set of fluids is chosen, delegates are updated to point to selected instance, based on fluid id. So FluidStack.getFluid does not return instance that is passed in constructor, but one that is selected based on fluid name - in theory name lookup should always succeed.

Now, I already know there is other mod in that crash report that registers same fluid id, but I never was able to reproduce that crash, no matter which instance was selected.

Considering access restriction on most parts of this flow, it can be only broken by irresponsible use of reflection/coremods, some weird level.dat corruption that causes delegate rebind to update value to null and/or something causing delegate rebind to not run.

EDIT: There is probably no need (or even sane way) to protect against this crash, as registry is already in invalid state.

[pupnewfster]

Ya I haven't done much with the fluid registry personally so I had not realized FluidStack creation already ensures it is using the correct Fluid object.

[thiakil]

Fluid delegates help in a lot of cases, though I'm fairly sure I've seen edge cases for it failing. Just can't remember off the top of my head. Could be just with instance comparisons to the non-default fluid though.

As for getting it to trigger yours not being default, you need to have the other mod installed without yours, create/load the world, then save, add your mod, and reopen - then the other is used as the default.