Engineers Workshop

Engineers Workshop

7M Downloads

1.3.4-1.10.2 Client disconnected due to exception when closing the GUI while the workbench still has stored power.

JaSpr opened this issue ยท 21 comments

commented

Because version 1.3.2 was not allowing items to be transferred into the workshop, we updated our FTB Beyond server (and clients) to have 1.3.4.

Now, whenever we close the GUI for the Engineer's workbench, an index out of bounds error with a network call forces the client to disconnect with a fatal error.

[20:56:57] [Netty Client IO #1/ERROR] [FML]: 1: ASM: engineers.workshop.common.network.PacketHandler@726b0782 onClientPacket(Lnet/minecraftforge/fml/common/network/FMLNetworkEvent$ClientCustomPacketEvent;)V
[20:56:57] [Netty Client IO #1/ERROR] [FML]: NetworkEventFiringHandler exception
java.lang.IndexOutOfBoundsException: readerIndex(3) + length(1) exceeds writerIndex(3): UnpooledHeapByteBuf(ridx: 3, widx: 3, cap: 3)
VERSIONS

Minecraft 1.10.2
Forge 12.18.3.2281
Pack: FBT Beyond 1.6.1 (with Eng.Workshop upgraded to 1.3.4)
Eng. Workshop 1.3.4-1.10.2

Expected behavior

Closing the GUI simply closes the GUI, regardless of power levels

Actual Behavior

If the workshop has any power (regardless of whether from RF or coal or solar) when you close the GUI the client is disconnected.

If you drain all the power before closing the GUI, there is no fatal error, nor any disconnect, and everything works as expected.

Link to Crashlog or Forgelog [If Applicable]

Index out of bounds exception. Potentially an off-by-one error.

https://gist.github.com/JaSpr/4ab7b07cfe11401785dd006d983294b6

Steps to reproduce [If Applicable]
  1. Give workbench any power (coal, RF, or solar)
  2. close the GUI
  3. crash/disconnect
commented

Still running into this issue on 1.36. In my case I've got a simple engineer's workshop setup with a set of 4 furnaces. There seems to be a chance to crash anytime one of the furnaces is active.

https://pastebin.com/vEHKtqGu

commented

So... trying to do what I can to help, Unfortunately, as of 7 days ago a commit went through that nuked the relevant files so I don't even know if this is at all relevant still. Also, this all comes from my understanding of network code and about an hour of looking at your code with minimal understanding of minecraft's code... so take it with a grain of salt.

As of commit bd605bb it looks like this may be an issue of the server sending a packet intended to go to the table the player has open, and the player no longer having that table open.

If I have a table open and the server knows this, then when the server sends me an update DataWriter.getWriter will write the packet id and no table position. I have to assume that somewhere down the line other information also gets written, but that's not relevant.
The server sends me this packet. Before I receive this packet, I close the table. Now the server has sent me a packet assuming I have the interface open, but I don't have it open.
I get the packet and ultimately that packet data gets passed into PacketHandler.onPacket. onPacket reads the enum, which says that I have a table open. It checks to see if the container I have open is of the containerTable type, finds that it's not (because I don't have one open) and so it doesn't populate container table. Table remains null. Because table is null, it tries to get the cable coordinates from the packet but this was a packet assuming that a table was open. That data isn't there and the attempt to read it fails.

Or I entirely missed something important.

commented

@Darinth , That change was intentional and should have no effect on this bug, That was there because there was the occasional packet that would be sent when a table was null (either not loaded, or just broken, etc.) that would bug out. That line is the same as the else, because what happens before the else is setting table not to be null.

commented

@Darinth , I'll re implement that else to see if it changes anything. Thank you for the long explanation (It's nice when people actually try to help instead of just mindlessly posting a bug report). We have almost completely finished 1.10 builds (unless there's a massive bug, which I don't consider this because of it's infrequency), so it'll be under the 1.12 builds

commented

I do try. Hopefully it'll fix the issue for 1.12 and I'll get to enjoy it when I move on to putting a 1.12 pack together. Sounds like this still leaves you with another bug which still needs a fix though.

commented

Actually, it seems as if another approach has been taken by the person who ported it to 1.12. That code is now in DataPacket.java, and it checks to see if the packet has sent along a position, which means it won't try to read those extra pieces that don't exist. I'm going to close this issue as it's been fixed for 1.12, and not many packs/servers are still running 1.10.

IF this becomes highly requested, I'll put out another version for 1.10 that has this fix, but most modpacks won't update to add it, and you'd need to manually update client and possibly the server (most likely not)

commented

If I'm right, the issue is in commit 1b3559f.

PacketHandler.java changed.

}
} else {
int x = dr.readSignedInteger();
int y = dr.readSignedInteger();

}
}
if (table == null) {
int x = dr.readSignedInteger();
int y = dr.readSignedInteger();

commented

@JorVaCoding Sorry that I was one of the ones that mindlessly posted just another crashlog. I'm no coder so that is pretty much all I can contribute along with info on what was going on at the time in game. As to the 1.10 fix, would just like to add my 2 cents in favor of the fix for 1.10 in the "IF this becomes highly requested" category, but only if it's not a pain the ass to make it happen on 1.10. Myself along with many others still play older packs that include this mod. Heck, I'm still replaying Crash Landing on 1.6.4 as well as Golem Factory a gem I totally missed back then. Figure it's better to put in the request while a version is still a little bit of a thing rather then once it's antiquated and clearly not something that should be asked. Again apologies for being a lemming with my crash report, didn't mean to offend.

commented

I doubt JorVa minds people who can't help. The reality is that most people are not coders. Mankind thrives on division of labor; we each have our specilized talents.

If you want to get this fix yourself, you can download the 1.3.6 version, the 1.3.3 version. Open them up in your choice of archive handling program (I use 7-zip) and copy the packethandler.class file from the 1.3.3 version into the same location in the 1.3.6 version. With JorVa's permission, I can post up my copy of the zip file that I've done so with but I don't do that without author permission.

commented

I am at home on christmas vacation and I only got my phone here at home, so a bit hard to code. If you can link me to a modified build with the old packet handler I can release it on Curse Forge @Darinth ๐Ÿ‘

commented

https://www.dropbox.com/s/0e25jw9xzcayxxm/EngineersWorkshop-1.3.6-1.10.2UnofficialDarinth.jar?dl=0

That's the 1.3.6 jar file with the packethandler.class file from 1.3.3.

commented

Once approved the download should be up here: https://minecraft.curseforge.com/projects/engineers-workshop/files/2516567.

Many thanks @Darinth

commented

I was able to crash doing this, then when I broke the table and replaced it, I couldn't crash. This might be a bug with some of the networking code we changed in this update to allow a better render, but I can't really reproduce it well enough to find the error.

It's something with the packet from the server not sending the coords/power to the player fully.

commented

Also cant reproduce this with a different world that had an old table, I'll keep seeing if I can get it to crash again.

commented

Please ignore my previous (deleted) comment. I had misread your comment on the other ticket. Thanks for your feedback!

commented

I have 2 tables setup, only the one with crafting tables, auto crafting and auto transfer upgrades crashes.

The table with furnace, fuel efficiency and speed upgrades does not.

commented

I placed a table, added 4x crafting tables to it and crash 50% of the time opening/closing the UI.

Did not have any upgrades nor fuel, simply 4x crafting tables.

commented

Have had a player complain of the same issue on 1.3.5, within the AllTheMods modpack.

https://pastebin.com/pZfd1sKw

commented

Forge 12.18.3.2316
Sponge 1.10.2-2281-5.2.0-BETA-2401 (I know, I know)
MC 1.10.2
Curse modpack "FTB Beyond" 1.9(Beta)
is crash on server: yes [4G ram max. Allocating avg. 2.3G for 10-14 players. Avg. Tps 16.5-17]
is crash on single: No/can't reproduce

Client latest:
https://gist.github.com/skittishtrigger/4760b6866a851c4fbb3620aefffa622d

Issue: [On a server]
Place workshop table
open gui
Insert furnace
Insert coal
insert cobble stone
Watch for a second to make sure it works
Hit escape to exit to exit gui
Client crashes

Accidentally posted this over in IE's page. Hope another log helps.

commented

Just another snippet...
FTB Beyond: v1.10.0
Forge: 12.18.3.2316
Enginers Workshop: 1.35

Playing a lan game with the boy, he disconnects frequently, finally had it happen to me, lost connection to the internal server(wierd?). This snippet is from the most recent incident, the client didn't disconnect (had to force close, this is another issue though, hard to "quit" out of this pack). It seems to happen randomly, no matter the fuel levels as mentioned above, it can work flawlessly for hours, then boom.

https://pastebin.com/yBgnpSmN

commented

I have a modpack where it happens to multiple people. I can give you access to the server if you wanna check there.