Repository navigation
Fix Memory release not correctly in Multipart Decoder - #11188
Conversation
|
This does not fix #11174 |
|
Have you tested it ? If so, then, I don't understand the issue correctly. Does it concerned the "offered" chunks ? If so, then in Netty, the standard http codec handler does that for you. This is not the job of the MultipartDecoder. So, I was believing that it was the allocated data from the MultipartDecoder. As it used the same Pool allocator than original chunk, they could be Direct Memory too. And therefore, giving your issue. |
|
@fredericBregier I'll post a reply in my issue |
|
Notice: I think there is something wrong with this undecodedChunk when we end the offer part. It might have a refCnt > 1. |
|
@fredericBregier I'll try exercising the app with this change and report back |
dfccd9e to
80088bf
Compare
|
@jameskleeh Sorry, wrong commit again (too late, I need to sleep). If you clone it, redo it please. |
|
@fredericBregier This logic doesn't get executed in my example. The issue is the Edit: That fixes the issue for 4.1.59, however it does not for 4.1.63 |
80088bf to
1f9ec02
Compare
|
@jameskleeh I guess I found the reason. Can you recheck ? |
1f9ec02 to
42e0a14
Compare
|
@normanmaurer I agree, that's why I remove the second one in destroy. |
|
@fredericBregier not sure if there is just some miss-understanding. If there is any |
|
@normanmaurer 2 possibilities:
WDYT? |
42e0a14 to
e8e8610
Compare
|
@normanmaurer I opt for the moment on the second solution (keeping your code), but I believe the |
e8e8610 to
a101b9b
Compare
|
@normanmaurer I have completed the last commit with 2 things, in addition to remove all while loop
|
|
@normanmaurer So the tests passed with no more Memory leak. |
695ae4a to
b7b1476
Compare
|
@fredericBregier can you please squash and adjust the commit message ? |
Motivation: 2 years ago a change remove the default clearing of all HttpData, whatever they are disk based or memory based. A lot of users were probably releasing HttpData directly, so there was no issue. But now, it seems, and as the Javadoc said, that `decoder.destroy()` shall clean up also Memory based HttpData, and not only Disk based HttpData as currently. Change: Add in `destroy()` method the necessary code to release if necessary the underlying Memory based HttpDatas. Change one Junit Test (using Mixed, Memory and Disk based factories) in order to check the correctness of this behavior and to really act as a handler (releasing buffers or requests). Modify one Junit core to check validity when a delimiter is present in the Chunk but not CRLF/LF (false delimiter), to ensure correctness. Result: No more issue on memory leak Note that still the List and the Map are not cleaned, since they were not before. No change is done on this, since it could produce backward issue compatibility. Fix issues netty#11175 and netty#11184
b7b1476 to
6d8e346
Compare
|
@normanmaurer Done, CI/CD rechecking now ;-) |
normanmaurer
changed the title
normanmaurer
changed the title
|
@fredericBregier thanks a lot! |
Motivation: 2 years ago a change remove the default clearing of all HttpData, whatever they are disk based or memory based. A lot of users were probably releasing HttpData directly, so there was no issue. But now, it seems, and as the Javadoc said, that `decoder.destroy()` shall clean up also Memory based HttpData, and not only Disk based HttpData as currently. Change: - Add in `destroy()` method the necessary code to release if necessary the underlying Memory based HttpDatas. - Change one Junit Test (using Mixed, Memory and Disk based factories) in order to check the correctness of this behavior and to really act as a handler (releasing buffers or requests). - Modify one Junit core to check validity when a delimiter is present in the Chunk but not CRLF/LF (false delimiter), to ensure correctness. Result: No more issue on memory leak Note that still the List and the Map are not cleaned, since they were not before. No change is done on this, since it could produce backward issue compatibility. Fix issues #11175 and #11184
Motivation: 2 years ago a change remove the default clearing of all HttpData, whatever they are disk based or memory based. A lot of users were probably releasing HttpData directly, so there was no issue. But now, it seems, and as the Javadoc said, that `decoder.destroy()` shall clean up also Memory based HttpData, and not only Disk based HttpData as currently. Change: - Add in `destroy()` method the necessary code to release if necessary the underlying Memory based HttpDatas. - Change one Junit Test (using Mixed, Memory and Disk based factories) in order to check the correctness of this behavior and to really act as a handler (releasing buffers or requests). - Modify one Junit core to check validity when a delimiter is present in the Chunk but not CRLF/LF (false delimiter), to ensure correctness. Result: No more issue on memory leak Note that still the List and the Map are not cleaned, since they were not before. No change is done on this, since it could produce backward issue compatibility. Fix issues netty#11175 and netty#11184
Motivation:
2 years ago a change remove the default clearing of all HttpData, whatever
they are disk based or memory based.
A lot of users were probably releasing HttpData directly, so there was no issue.
But now, it seems, and as the Javadoc said, that
decoder.destroy()shall clean upalso Memory based HttpData, and not only Disk based HttpData as currently.
Change:
Add in
destroy()method the necessary code to release if necessarythe underlying Memory based HttpDatas.
Change one Junit Test (using Mixed, Memory and Disk based factories)
in order to check the correctness of this behavior and to really act
as a handler (releasing buffers or requests).
Modify one Junit core to check validity when a delimiter is present in the Chunk
but not CRLF/LF (false delimiter), to ensure correctness.
Result:
No more issue on memory leak
Note that still the List and the Map are not cleaned, since they were not
before. No change is done on this, since it could produce backward issue compatibility.
Fix issues #11175 and #11184