Minecraftforge: SimpleNetworkWrapper reply mechanism not threadsafe

[DaveyBiggers]

SimpleChannelHandlerWrapper.channelRead0 contains the following code:

        REPLY result = messageHandler.onMessage(msg, context);
        if (result != null)
        {
            ctx.channel().attr(FMLOutboundHandler.FML_MESSAGETARGET).set(FMLOutboundHandler.OutboundTarget.REPLY);
            ctx.writeAndFlush(result).addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE);
        }

Unfortunately, as of 1.8, this code is executed on the netty io thread, which (unless I'm misunderstanding) opens up a possible race condition with the SimpleNetworkWrapper.sendTo... methods, meaning the outbound target can be changed while the message is en route.

This explains behaviour I've been seeing lately where calling sendTo(IMessage message, EntityPlayerMP player) from the server occasionally fails with an NPE due to missing originator information in the packet. It seems that the outbound target in the channel attributes is occasionally accidentally getting set to "REPLY" instead of "PLAYER". If I make sure all my onMessage() methods return NULL then this stops happening.

(This was using Forge 1.11.2-13.20.0.2228 but the relevant code hasn't changed since then.)

[diesieben07]

The whole reply system is pretty broken right now anyways, because you can't reasonably return a response most of the time, since you need to wait for your code to be scheduled on the main thread. onMessage should have the possibility to return a future instead.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

[bs2609]

Perhaps this can be looked at for 1.13?

[HenryLoenwind]

One sneaky way to fix that would be to split the handler into two methods. One will be called in the network thread and has a boolean return value. If it returns false, the second one, with the signature as is now, will be called on the main thread where returning an IMessage is fine.

This also takes care of the fact that 99% of all message handlers will have to run on the main thread anyway and that each and every modder has to find a solution for this themselves atm.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[williewillus]

this is still relevant for all the reasons described above, and should either be fixed or removed (probably the latter) in 1.13

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.