Massive packet leakage
cpw opened this issue ยท 15 comments
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.
As an FYI @SpaceToad, this bug will cause epic log saturation on the next forge update. I really suggest you fix it..
Thanks for the report @cpw. I'm working on it high priority - hopefully with a release very quickly.
Arf - I can tell were the original error is coming from though (http://www.minecraftforge.net/wiki/Tutorials/Packet_Handling): :-(
@SpaceToad i'm pretty sure the majority of 1.7 mods used that tutorial and have the same problem now
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..
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?
@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?
@SpaceToad, looks like the travis files for compiling are on 6.0 but not on 5.0 so it can't compile
@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.
@SpaceToad ok wasn't sure if this was intended or not, but then you should probalby setup travis to ignore the 5.0 branch
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..
@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.
@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
@AEnterprise no, not really lag- although in a world with a lot of pipes, your client will eventually run out of memory and crash..
@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