Wynntils

Wynntils

Mods
611k Downloads

[2.0.3] exception in container query

magicus opened this issue ยท 10 comments

commented 
[15:59:18] [Render thread/ERROR] (EventBus) Exception caught during firing event: Cannot invoke "com.wynntils.handlers.container.scriptedquery.QueryStep.getVerification()" because "this.currentStep" is null
    Index: 1
    Listeners:
        0: NORMAL
        1: ASM: com.wynntils.handlers.container.ContainerQueryHandler@6683ff7b onMenuOpened(Lcom/wynntils/mc/event/MenuEvent$MenuOpenedEvent;)V
        2: ASM: com.wynntils.models.character.CharacterSelectionModel@6d2ecbad onMenuOpened(Lcom/wynntils/mc/event/MenuEvent$MenuOpenedEvent;)V
        3: ASM: com.wynntils.models.worlds.WorldStateModel@36ef033c onMenuOpened(Lcom/wynntils/mc/event/MenuEvent$MenuOpenedEvent;)V
        4: ASM: com.wynntils.models.containers.LootChestModel@2fca32a3 onMenuOpened(Lcom/wynntils/mc/event/MenuEvent$MenuOpenedEvent;)V
        5: ASM: com.wynntils.models.seaskipper.SeaskipperModel@3adae4c1 onMenuOpened(Lcom/wynntils/mc/event/MenuEvent$MenuOpenedEvent;)V
        6: ASM: com.wynntils.models.emeralds.EmeraldModel@4214711a onMenuOpened(Lcom/wynntils/mc/event/MenuEvent$MenuOpenedEvent;)V
java.lang.NullPointerException: Cannot invoke "com.wynntils.handlers.container.scriptedquery.QueryStep.getVerification()" because "this.currentStep" is null
    at com.wynntils.handlers.container.scriptedquery.ScriptedContainerQuery.verifyContainer(ScriptedContainerQuery.java:56)
    at com.wynntils.handlers.container.ContainerQueryHandler.onMenuOpened(ContainerQueryHandler.java:110)
    at com.wynntils.handlers.container.__ContainerQueryHandler_onMenuOpened_MenuOpenedEvent.invoke(.dynamic)
    at net.minecraftforge.eventbus.ASMEventHandler.invoke(ASMEventHandler.java:73)
    at net.minecraftforge.eventbus.EventBus.post(EventBus.java:315)
    at net.minecraftforge.eventbus.EventBus.post(EventBus.java:296)
    at com.wynntils.core.events.EventBusWrapper.post(EventBusWrapper.java:54)
    at com.wynntils.core.WynntilsMod.postEvent(WynntilsMod.java:69)
    at com.wynntils.core.events.MixinHelper.post(MixinHelper.java:21)
    at net.minecraft.client.multiplayer.ClientPacketListener.handler$zbd000$wynntils$handleOpenScreenPre(ClientPacketListener.java:2919)
    at net.minecraft.client.multiplayer.ClientPacketListener.handleOpenScreen(ClientPacketListener.java)
    at net.minecraft.network.protocol.game.ClientboundOpenScreenPacket.handle(ClientboundOpenScreenPacket.java:37)
    at net.minecraft.network.protocol.game.ClientboundOpenScreenPacket.handle(ClientboundOpenScreenPacket.java:11)
    at net.minecraft.network.protocol.PacketUtils.method_11072(PacketUtils.java:22)
    at net.minecraft.util.thread.BlockableEventLoop.doRunTask(BlockableEventLoop.java:156)
    at net.minecraft.util.thread.ReentrantBlockableEventLoop.doRunTask(ReentrantBlockableEventLoop.java:23)
    at net.minecraft.util.thread.BlockableEventLoop.pollTask(BlockableEventLoop.java:130)
    at net.minecraft.util.thread.BlockableEventLoop.runAllTasks(BlockableEventLoop.java:115)
    at net.minecraft.client.Minecraft.runTick(Minecraft.java:1174)
    at net.minecraft.client.Minecraft.run(Minecraft.java:801)
    at net.minecraft.client.main.Main.main(Main.java:237)
    at net.fabricmc.loader.impl.game.minecraft.MinecraftGameProvider.launch(MinecraftGameProvider.java:468)
    at net.fabricmc.loader.impl.launch.knot.Knot.launch(Knot.java:74)
    at net.fabricmc.loader.impl.launch.knot.KnotClient.main(KnotClient.java:23)
    at net.fabricmc.devlaunchinjector.Main.main(Main.java:86)
    at dev.architectury.transformer.TransformerRuntime.main(TransformerRuntime.java:219)
commented

I think I found the root cause, but I don't have enough knowledge to fix this. Please verify if I am correct, if so, this should be an easy fix.

  • ContainerQueryHandler#raiseError calls currentStep#onError which in turn set's it's own currentStep to null.
  • ContainerQueryHandler#raiseError then calls endQuery, stopping the query, but still having lastStep set.
  • New menu gets opened, the fallback code (ContainerQueryHandler line 107-121) is ran.
  • lastStep#verifyContainer is called but currentStep is null now. The exception happens.
commented

I have unassigned myself now.

commented

Debugging seems to confirm my theory.

commented

I don't understand how that could happen..?

        if (currentStep == null && lastStep != null) {
            // We're in a possibly bad state. 
...
            // Now say we're completely finished with the last query
            lastStep = null;
            return;
        }

        // Are we processing a query?
        if (currentStep == null) return;

        if (currentStep.verifyContainer(e.getTitle(), e.getMenuType())) {
commented

Oh, I see...

commented

When I designed the code, I assumed we would always get distinct query step objects, and that they would be immutable and self-sustaining. Now when ScriptedContainerQuery is the ContainerQueryStep, we will always assume that we call verifyContainer on ourself on the current step. The last step was the very same object, but now it has changed.

commented

I think the solution is to just get rid of the lastStep stuff. It was only there to try to diagnose timing issues; I'm not sure it is even relevant.

commented

I think the solution is to just get rid of the lastStep stuff. It was only there to try to diagnose timing issues; I'm not sure it is even relevant.

In the past, I have seen a lot of cases of failed processing getting stuck and "bugging" inventories, leaving them in a bad state. So seeing that code definitely felt like an improvement, although, it never worked in production.

commented

So, what would it take to keep this? Or is it completely an out-dated system?

commented

Fixed by #1741