Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the haproxy message mem leak issue #9250

Merged
merged 9 commits into from Jun 24, 2019

Conversation

Projects
None yet
3 participants
@qeesung
Copy link
Contributor

commented Jun 17, 2019

Motivation:

HAProxyMessage should be released as it contains a list of TLV which hold a ByteBuf, otherwise, it may cause memory leaks.

Modification:

Let HAProxyMessage inherit ByteBufHolder and override ByteBufHolder's method so that it can release itself as well as TLV list

Result:

Fixes #9201

@netty-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

Can one of the admins verify this patch?

if (header.readableBytes() < 16) {
throw new HAProxyProtocolException(
"incomplete header: " + header.readableBytes() + " bytes (expected: 16+ bytes)");
}

ByteBuf rawContent = header.retain();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

imho we should call retain as last method to ensure we can not leak

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done :-)

if (header == null) {
static HAProxyMessage decodeStringHeader(ByteBuf header) {
String headerStr = header.toString(CharsetUtil.US_ASCII);
if (headerStr == null) {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

toString(...) will never return null so this does not look correct. Maybe it should check if the buffer is empty ?

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done


@Override
public HAProxyMessage copy() {
List<HAProxyTLV> copiedTLVs = new LinkedList<HAProxyTLV>();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

why not an ArrayList to reduce the GC ?

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done


@Override
public ByteBufHolder duplicate() {
List<HAProxyTLV> duplicatedTLVs = new LinkedList<HAProxyTLV>();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

why not an ArrayList to reduce the GC ?

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done


@Override
public ByteBufHolder retainedDuplicate() {
List<HAProxyTLV> retainedDuplicatedTLVs = new LinkedList<HAProxyTLV>();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

why not an ArrayList to reduce the GC ?

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done

}

@Override
public ByteBufHolder retainedDuplicate() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

Should return HAProxyMessage .

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done

}

@Override
public ByteBufHolder duplicate() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 17, 2019

Member

Should return HAProxyMessage .

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 18, 2019

Author Contributor

Done

private static final HAProxyMessage V2_LOCAL_MSG = new HAProxyMessage(
HAProxyProtocolVersion.V2, HAProxyCommand.LOCAL, HAProxyProxiedProtocol.UNKNOWN, null, null, 0, 0);

public final class HAProxyMessage extends DefaultByteBufHolder {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 18, 2019

Member

imho we should not extend DefaultByteBufHolder but AbstractReferenceCounted. There is no need to hold a reference to the "original" ByteBuf here.

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 20, 2019

Author Contributor

I like this idea, I think if HAProxyMessage inherits AbstractReferenceCounted, then maybe we can make the implementation a bit simpler. Just overwrite the AbstractReferenceCounted.deallocate() method and release the TLV list in this method, like this:

class HAProxyMessage extends AbstractReferenceCounted {
    @Override
    public HAProxyMessage retain() {
        return (HAProxyMessage) super.retain();
    }
    // overwrite other methods...

    @Override
    protected void deallocate() {
       for (HAProxyTLV tlv : this.tlvs){
           tlv.release();
       }
    }
}

In this case, the TLV list will be released together when HAProxyMessage is released, but when HAProxyMessage refCnt is increased, the TLV refCnt will not increase with HAProxyMessage.

@normanmaurer What do you think of this? Will it cause other additional problems?

By the way, I found that this is also the case in DNSMessage:

@Override
protected void deallocate() {
clear();
final ResourceLeakTracker<DnsMessage> leak = this.leak;
if (leak != null) {
boolean closed = leak.close(this);
assert closed;
}
}

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 20, 2019

Member

@qeesung good question...Let me think about it but in general I would say it is fine.

This comment has been minimized.

Copy link
@qeesung

qeesung Jun 20, 2019

Author Contributor

@normanmaurer It's done in this way. Could you please review it again :-)?

@qeesung qeesung force-pushed the qeesung:haproxy_mem_leak branch from 4d22e59 to 49ad580 Jun 20, 2019

@normanmaurer
Copy link
Member

left a comment

Can you also add a test ?

@normanmaurer
Copy link
Member

left a comment

one nit and then we are ready...

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

@netty-bot test this please

@qeesung

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

@normanmaurer Three more changes have been added. Cloud you please review them again?

  1. Remove all static unknown HAProxyMessages and replaced with calling static methods to generate a new one. This avoids the problem of users releasing static unknown HAProxyMessages.
  2. After checking the validity of all the construction parameters of HAProxyMessage, the ResourceLeakDetector is used to trace the message, which can avoid the memory leak alarm when the constructor throws an exception.
  3. Fix memory leaks that HAProxyMessage is not released in test cases
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

@netty-bot test this please

1 similar comment
@qeesung

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

@netty-bot test this please

@qeesung

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

@normanmaurer It seems that a test case failed because of address conflict. How can I trigger the Jenkins build again?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@netty-bot test this please

@qeesung

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

@normanmaurer Do you have time to help check why there is address conflict in Java 11? I can't find any problems. Is it possible that the Jenkins machine has a TCP connection in the TIME_WAIT state?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

This is the "in-vm" transport.. Not sure why this happens. I think it is safe to just merge this PR and I will investigate.

@normanmaurer normanmaurer added this to the 4.1.37.Final milestone Jun 24, 2019

@normanmaurer normanmaurer merged commit 712077c into netty:4.1 Jun 24, 2019

2 of 3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@qeesung thanks a lot!

normanmaurer added a commit that referenced this pull request Jun 24, 2019

Fixed the haproxy message mem leak issue (#9250)
Motivation:

HAProxyMessage should be released as it contains a list of TLV which hold a ByteBuf, otherwise, it may cause memory leaks.

Modification:

- Let HAProxyMessage extend AbstractReferenceCounted
- Adjust tests.

Result:

Fixes #9201

@qeesung qeesung deleted the qeesung:haproxy_mem_leak branch Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.