BuildCraft|Core

BuildCraft|Core

7M Downloads

Massive packet leakage

cpw opened this issue ยท 15 comments

commented

5.0.5 has a serious packet leakage. This is because the netty pipeline is incorrectly configured, and instead of having handlers which generate messages (codecs) followed by handlers which process the messages, there is simply PacketHandler, which is both a codec and a message handler. Because of this, BuildCraft is not "reading" it's messages (since they're being processed during codec transformation), and as a result, the netty channel is accumulating BC messages at a significant rate.

PacketHandler and BuildCraftChannelHandler should be independent registrations to the netty channel. PacketHandler probably should be broken apart to handle individual message types, but could also handle ALL types if that's easier.

FYI, code inspection indicates this is a problem @ 6.0.x and 6.1.x as well.

commented

As an FYI @SpaceToad, this bug will cause epic log saturation on the next forge update. I really suggest you fix it..

commented

Thanks for the report @cpw. I'm working on it high priority - hopefully with a release very quickly.

commented

Arf - I can tell were the original error is coming from though (http://www.minecraftforge.net/wiki/Tutorials/Packet_Handling): :-(

commented

@SpaceToad i'm pretty sure the majority of 1.7 mods used that tutorial and have the same problem now

commented

Yes, I wish I'd realised the problem sooner. :-( the fix should be pretty straightforward. Make packethandler implement simplechannelhandleradapter. Register it as a second element on the pipeline..

commented

Generally speaking, I feel like I need to dive into netty more deeply. Trying to avoid gaining that piece of knowledge hasn't proven to be very good. One of the difficulty that I keep bumping into is to understand the separation between netty concepts and Forge helpers.

BTW - I assume you're talking about SimpleChannelHandlerWrapper - not SimpleChannelHandlerAdapter, right?

commented

@cpw I run a first tentative fix following the understanding I have at this stage. First shallow testing indicates that it should work. Do you mind running a quick review to verify that the leak you identified should be fixed?

commented

@SpaceToad, looks like the travis files for compiling are on 6.0 but not on 5.0 so it can't compile

commented

@AEnterprise of course it can't - travis, and style checks in general have been introduced on the 6.0 branch. This will not get backported.

commented

@SpaceToad ok wasn't sure if this was intended or not, but then you should probalby setup travis to ignore the 5.0 branch

commented

I don't have testing facilities at this second, but I reviewed your patch and it should fix it perfectly. The problem is caused by acting on packets directly in a codec subclass, rather that passing the packets forward to a handler. You appear to have fixed that.. I am pushing a new forge build with the leak logging in place. If you uptake forge 1102 and try old and new, you'll see the bug VERY quickly (on 5.0.5) and hopefully not at all on your fix..

commented

@cpw thanks for the confirmation. I'll wait and upgrade Forge to confirm and will do a synchronized 5.0 / 6.0 release once everything is cleared.

commented

@SpaceToad @cpw is this causing alot of lag? cause the offical modpack is locked from updates cause we want to use a dev pack to test things out first. But if this is causing alot of lag we could update the pack for it

commented

@AEnterprise no, not really lag- although in a world with a lot of pipes, your client will eventually run out of memory and crash..

commented

@cpw, thanks i'll do an update of the pack then to make sure it won't cause any issues, 6.0 branch is pretty stable so shouldn't cause any issues