ByteBuf.release() was not called while doing a multipart upload #4837
Assignees
Milestone
Comments
Is that initiating concurrent uploads or are they happening 1 after the other? |
That's concurrent. |
|
@kindlychung Based on what I've read, |
My mistake. The command looked like it would likely send the requests one after the other. Thanks for the feedback. |
|
I cannot trigger the The |
|
Here is the heap chart from visualvm:
It doesn't seem the max heap size has been exceeded, which is really weird. |
|
@kindlychung Why are you copying the input stream to a temp file, just to open a stream from the temp file to upload? You should be able to use the stream directly. |
Yeah, that's not necessary at all (the repo was prepared by one of my colleagues). But I can confirm the same |
|
@kindlychung Can you update the example project to represent the code that is producing the error? |
Sure, I have simplified the code base and pushed to the main branch. |
|
@kindlychung After looking at your changes I still am not seeing the input stream being closed https://github.com/gemvision/micronaut_image_test/blob/main/src/main/java/micronaut_image_test/SendImageController.java#L57. Are you certain that is happening through the put request? |
That input stream is from the |
|
@kindlychung You need to close the stream. I don't know when is the time to do that given I'm unfamiliar with the S3 api, however not closing it will result in the problem you're seeing. When you are able to figure out the time to close the stream that allows the file to still be uploaded, and the issue is still happening, please update this issue and the example code. |
@jameskleeh I've added the finally clause to close the stream. Never mind the 0-byte blob, the OutOfMemoryError still happens. |
|
also getting this issue while handling multipart upload with CompletedFileUpload |
|
Over the weekend i did some tests on the server, and the leaks seem to occur when FileAlreadyExistsException is caught. The request is processed correctly, but something does not get closed i'm guessing |
|
We are experiencing similar behaviour using Java 11 and Micronaut 2.3.2. We've managed to reproduce the behaviour locally with our application running in docker with limited memory and using the We set the jvm flag |
|
@jaksah can you provide the example application? |
|
@kalgecin If this line |
|
isn't the correct behavior to release the resources after the method returns? What if my logic does not require to process the file at certain times? Say for example i check the file size for limits, or something else. |
|
@graemerocher Here's an example application: https://github.com/jaksah/micronaut-multipart-memory-leak |
@kalgecin The logic that processes the return of the method has no knowledge of any file arguments, etc.. The data is released as soon as you use it, which is the correct approach given the data can be released sooner compared to the end of the method. Not to mention methods that return reactive streams return immediately before the file is read |
|
@jaksah In that example application you are also not reading the file, thus the same thing applies |
|
Thanks for clarifying @jameskleeh. I've updated my example application to have the controller read the bytes of the file but also making the route secured and managed to reproduce the LEAK by sending unauthenticated requests. How do we make sure to release the data in this case? |
|
@jaksah I'll take a look. You shouldn't be responsible for doing so in that case |
|
Is it not possible to try and close all buffers on method return? |
unfortunately even after adding code to read the bytes on method entry, before any exceptions can be thrown, still getting this log |
|
We managed to find a workaround that does not trigger the ByteBuf.release() exception any more, which basically reads the whole body inside the controller instead of using argument binding. More or less like this: @Post(value = "/upload", consumes = MediaType.MULTIPART_FORM_DATA)
public Single<HttpResponse> upload(
@Body MultipartBody body
) {
return readMultipartBody(body)
.flatMap(...)
}
Single<RequestDTO> readMultipartBody(final MultipartBody body) {
return Single.create(emitter -> body.subscribe(new Subscriber<>() {
private Subscription s;
private RequestDTO.Builder requestBuilder = RequestDTO.builder();
private final List<Exception> errors = new ArrayList<>();
@Override
public void onSubscribe(final Subscription s) {
this.s = s;
s.request(1);
}
@Override
public void onNext(final CompletedPart completedPart) {
try {
// Always read the data to make sure it's released
byte[] data = completedPart.getBytes();
// logic to append information to the builder
} catch (IOException e) {
log.warn(
"Unable to read part " + completedPart.getName() + " from multipart/form-data request", e);
errors.add(e);
}
s.request(1);
}
@Override
public void onError(final Throwable t) {
emitter.onError(t);
}
@Override
public void onComplete() {
if (errors.isEmpty()) {
emitter.onSuccess(requestBuilder.build());
} else {
Throwable error = new HttpStatusException(HttpStatus.BAD_REQUEST, "Bad request");
for (Exception e : errors) {
error.addSuppressed(e);
}
emitter.onError(error);
}
}
}));
} |
|
@jaksah your solution seems to work for me too. It would seem there's a problem with the way micronaut handles the body of multipart when using |
This case has been resolved in 2.5 |
|
We just had the same problem on micronaut 2.5.13 and 3.0.0, stress testing service with 100-200 workers doing multipart fileupload of ~100KB sized files. To trigged it, you'll need at least few dozen parallel requests. Netty's resource leak diagnostics points out to FormDataHttpContentProcessor:119. @jaksah solution worked, I guess that using @Body MultipartBody bypasses FormDataHttpContentProcessor. |
|
@vnesek do you have a reproducer? |
|
I'll try to post a sample project in next few days. |
|
Same as @vnesek here. Using Working on a reproducer... |
|
@graemerocher I created a reproducer here: https://github.com/caiolopes/micronaut-file-upload There are two endpoints, one using |
yawkat
added a commit
that referenced
this issue
Nov 12, 2021
Attribute parts were not released properly when combined with streaming another part. This patch adds the attribute part to the request so that the request handles cleanup. We can't release the data immediately, because the route method may still expect it as a ByteBuffer parameter. There is no unit test for this, because this only fixes a bytebuf leak, which can't be tested in our current setup. Fixes #4837
jameskleeh
pushed a commit
that referenced
this issue
Nov 12, 2021
Attribute parts were not released properly when combined with streaming another part. This patch adds the attribute part to the request so that the request handles cleanup. We can't release the data immediately, because the route method may still expect it as a ByteBuffer parameter. There is no unit test for this, because this only fixes a bytebuf leak, which can't be tested in our current setup. Fixes #4837
dstepanov
pushed a commit
to konrad-kaminski/micronaut-core
that referenced
this issue
Nov 22, 2021
Attribute parts were not released properly when combined with streaming another part. This patch adds the attribute part to the request so that the request handles cleanup. We can't release the data immediately, because the route method may still expect it as a ByteBuffer parameter. There is no unit test for this, because this only fixes a bytebuf leak, which can't be tested in our current setup. Fixes micronaut-projects#4837
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Task List
Steps to Reproduce
for i in {0..300}; do curl --location --request POST 'http://localhost:8080/image/upload' --form 'file=@"/path/Videos/SampleVideo_1280x720_20mb.mp4"' -v & doneExpected Behaviour
All files should be uploaded to s3.
Actual Behaviour
Memory leak. Stacktrace:
This error always happens after an OutOfMemory exception (when a lot concurrent files are being uploaded):
Environment Information
Example Application
micronaut image test
The text was updated successfully, but these errors were encountered: