[Fixed] Potential memory leak

[theoriginalbit]

I am getting a potential memory leak detected when I was porting to my mod to the latest version. I cannot for the life of me figure out why.

 

Log output: http://pastebin.com/UHudKSSP

Networking code: https://github.com/theoriginalbit/MoarPeripherals/tree/master/src/main/java/com/theoriginalbit/moarperipherals/common/network

Sending the message: https://github.com/theoriginalbit/MoarPeripherals/blob/master/src/main/java/com/theoriginalbit/moarperipherals/common/tile/TileIronNote.java#L45

[jabelar]

I think the problem is probably with your output stream.  You keep creating new ones but never close them.  In you write NBT code you should add a finally section to your try-catch and close the stream there.

[theoriginalbit]

thank you, while that is an excellent catch that I missed (and will definitely keep in), I'm still getting the exact same issue, every 5 seconds it states the memory leak.

[jabelar]

I'm still suspicious of your write NBT code.  Did you know that ByteBufUtils has a writeTag() and readTag() method that allows you to directly put NBT into and out of a message buffer?  I suggest trying those.

[theoriginalbit]

I changed to use the ByteBufUtils, but since the NBT code is actually never called — the offending packet only uses int data — the stream should never be opened for the NBT read/write, meaning it isn't the cause of the leak.

[TheGreyGhost]

Hi

 

When I got this error message, it was because I was sending packets from the client but I hadn't registered a corresponding handler for that packet type on the server.  So the packets weren't being delivered.

 

The way I figured that out was to put a breakpoint in the forge code and look at what the 'this' packet was.

FMLProxyPacket::
processPacket()
                if (internalChannel.writeInbound(this))
                {
                    badPackets.add(this.channel);                                                // BREAKPOINT HERE, AND INSPECT this
                    if (badPackets.size() % packetCountWarning == 0)
                    {
                        FMLLog.severe("Detected ongoing potential memory leak. %d packets have leaked. Top offenders", badPackets.size());
                        int i = 0;
                        for (Entry<String> s  : Multisets.copyHighestCountFirst(badPackets).entrySet())

 

-TGG

[theoriginalbit]

What do you mean by not registering the handler? Is that not what this line is?

[TheGreyGhost]

Yeah, I would have thought so.

 

Except - now that I look more closely, perhaps it's not

 

<MessageIronNote.class, MessageIronNote.class> from INSTANCE.registerMessage(MessageIronNote.class, MessageIronNote.class, id++, Side.SERVER);

 

might not be the same as

 

<MessageIronNote, IMessage> from MessageIronNote extends MessageGeneric implements IMessageHandler<MessageIronNote, IMessage>

 

The templates on SimpleNetworkWrapper  caused me serious head damage for a while, still not 100% sure I understand how it works.  Maybe this is a similar problem.

 

-TGG

 

[shieldbug1]

I don't know if a IMessage can be its own IMessageHandler (I think I saw a pull for this get rejected on github).

 

Rather than having your MessageIronNote class implement IMessageHandler directly, try having a static nested class implement it instead.

public final class MyMessage implements IMessage
{
    //IMessage code
    ...
    
    public static final class Handler implements IMessageHandler<MyMessage, IMessage>
    {
        //Handler code
        ...
    }
}

And registering like this:

INSTANCE.registerMessage(MyMessage.Handler.class, MyMessage.class, id++, Side.SERVER);

[theoriginalbit]

Okay I just discovered my problem and it was the Side.SERVER, it is being sent from the server so it should be Side.CLIENT. though I swear I was getting a crash I'm no longer getting when I used Side.CLIENT. Thank you all for your help!

 

Yeah, I would have thought so.

 

Except - now that I look more closely, perhaps it's not

 

<MessageIronNote.class, MessageIronNote.class> from INSTANCE.registerMessage(MessageIronNote.class, MessageIronNote.class, id++, Side.SERVER);

 

might not be the same as

 

<MessageIronNote, IMessage> from MessageIronNote extends MessageGeneric implements IMessageHandler<MessageIronNote, IMessage>

 

The templates on SimpleNetworkWrapper  caused me serious head damage for a while, still not 100% sure I understand how it works.  Maybe this is a similar problem.

 

-TGG

Yeah in the realm of Generics they're definitely the same, MessageIronNote extends MessageGeneric which implements IMessage, so that would be valid. But you do give a good point that I should make things look a little more consistent, thanks.

 

I don't know if a IMessage can be its own IMessageHandler (I think I saw a pull for this get rejected on github).

 

Rather than having your MessageIronNote class implement IMessageHandler directly, try having a static nested class implement it instead.

public final class MyMessage implements IMessage
{
    //IMessage code
    ...
    
    public static final class Handler implements IMessageHandler<MyMessage, IMessage>
    {
        //Handler code
        ...
    }
}

And registering like this:

INSTANCE.registerMessage(MyMessage.Handler.class, MyMessage.class, id++, Side.SERVER);

Yeah I was suggested this change on IRC, while not actually a problem having them together, it might be worth splitting them for me, since I have developed MessageGeneric to be able to handle any packet I ever want to send.