Skip to content

Track memory attributes to avoid memory leak - #10432

Closed
mschneid wants to merge 3 commits into
netty:4.1from
mschneid:fix-memory-leak
Closed

mschneid wants to merge 3 commits into
netty:4.1from
mschneid:fix-memory-leak

Conversation

@mschneid

Copy link
Copy Markdown

Motivation:
In commit c354fa4 (#10209) the way attributes are decoded has changed. In HttpPostStandardRequestDecoder#decodeAttribute(ByteBuf, Charset) a buffer is allocated but may not be released while calling InterfaceHttpPostRequestDecoder#destroy().

HttpPostRequestDecoder decoder =
    new HttpPostRequestDecoder(new MemoryHttpDataFactory(false), request);
try {
    ...
} finally {
    decoder.destroy();
}

decoder.destroy() invokes DefaultHttpDataFactory#cleanRequestHttpData(HttpRequest), but as the memory attributes are not tracked, nothing is released.

Workaround:

HttpPostRequestDecoder decoder =
    new HttpPostRequestDecoder(new MemoryHttpDataFactory(false), request);
try {
    ...
} finally {
    decoder.getBodyHttpDatas()
        .forEach(ReferenceCounted::release);
    decoder.destroy();
}

Modification:
Track memory attributes so that they are released by calling HttpDataFactory#cleanAllHttpData() or HttpDataFactory#cleanRequestHttpData(HttpRequest).

Result:
Avoid memory leak

@netty-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

3 similar comments
@netty-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@netty-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@netty-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@normanmaurer

Copy link
Copy Markdown
Member

@mschneid can you add a unit test as well ?

Motivation:
In commit c354fa4 the way attributes are decoded has changed. In HttpPostStandardRequestDecoder#decodeAttribute(ByteBuf, Charset) a buffer is allocated but may not be released while calling InterfaceHttpPostRequestDecoder#destroy().

HttpPostRequestDecoder decoder =
    new HttpPostRequestDecoder(new MemoryHttpDataFactory(false), request);
try {
    ...
} finally {
    decoder.destroy();
}

decoder.destroy() invokes DefaultHttpDataFactory#cleanRequestHttpData(HttpRequest), but as the memory attributes are not tracked, nothing is released.

Workaround:
HttpPostRequestDecoder decoder =
    new HttpPostRequestDecoder(new MemoryHttpDataFactory(false), request);
try {
    ...
} finally {
    decoder.getBodyHttpDatas()
        .forEach(ReferenceCounted::release);
    decoder.destroy();
}

Modification:
Track memory attributes so that they are released by calling HttpDataFactory#cleanAllHttpData() or HttpDataFactory#cleanRequestHttpData(HttpRequest).

Result:
Avoid memory leak
@mschneid

Copy link
Copy Markdown
Author

@mschneid can you add a unit test as well ?

@normanmaurer Done

@normanmaurer

Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer

Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer

Copy link
Copy Markdown
Member

@mschneid there are unit test failures. PTAL

@normanmaurer

Copy link
Copy Markdown
Member

@netty-bot test this please

@mschneid

Copy link
Copy Markdown
Author

@normanmaurer should I squash this PR?

@normanmaurer

Copy link
Copy Markdown
Member

@mschneid let me have a look first. From a quick look I am not sure if this PR is 100 % correct. I will come back to you shortly

@normanmaurer

Copy link
Copy Markdown
Member

@mschneid so I did spent some time to look into this and the commit 9a701d5 seems to be not correct. Let me think about this a bit and come up with an alternative way of fixing the test-failures

@jameskleeh

Copy link
Copy Markdown
Contributor

It was decided this is a breaking change in my previous PR #9672

@mschneid

mschneid commented Feb 3, 2021

Copy link
Copy Markdown
Author

Any news on that?

@mschneid

Copy link
Copy Markdown
Author

This PR can be closed because the problem has been fixed with #11188.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants