Multipart handling errors #6705
Comments
|
thanks |
4 tasks
yawkat
added a commit
that referenced
this issue
Jan 3, 2022
Attributes returned by `postRequestDecoder.next()` are released twice when the decoder is destroyed: - Once because it's stored in the `bodyListHttpData` list - A second time because the `HttpDataFactory` keeps a copy around. This means that even if user code retains the attribute in question once, it may still be collected. In the test case, this issue is visible because the attribute is only parsed to a map later downstream in an IO thread, when the `FormDataHttpContentProcessor` has already completed. This patch uses `removeHttpDataFromClean` to avoid the duplicate release for HTTP data where it could happen. `removeHttpDataFromClean` is a no-op for data that isn't created by the `HttpDataFactory`, so this patch doesn't break in those cases. This patch risks exposing downstream buffer leaks: If the user code doesn't properly release attributes it retains, they would previously still have been collected because of this duplicate release. After this change, they may leak, potentially even leaving files on disk. However, imo this patch implements the "right" behavior, and we should deal with other issues as they appear. I've done some testing, but it's inconclusive. I ran `./gradlew check` with and without this patch, and in both cases, attribute files were retained in `/tmp`. Can't tell whether this patch introduced new leaks. Addresses #6705
|
fyi |
|
i think the hang is fixed by #6696, i will check once that is merged |
jameskleeh
pushed a commit
that referenced
this issue
Jan 3, 2022
* Fix double release of form attributes Attributes returned by `postRequestDecoder.next()` are released twice when the decoder is destroyed: - Once because it's stored in the `bodyListHttpData` list - A second time because the `HttpDataFactory` keeps a copy around. This means that even if user code retains the attribute in question once, it may still be collected. In the test case, this issue is visible because the attribute is only parsed to a map later downstream in an IO thread, when the `FormDataHttpContentProcessor` has already completed. This patch uses `removeHttpDataFromClean` to avoid the duplicate release for HTTP data where it could happen. `removeHttpDataFromClean` is a no-op for data that isn't created by the `HttpDataFactory`, so this patch doesn't break in those cases. This patch risks exposing downstream buffer leaks: If the user code doesn't properly release attributes it retains, they would previously still have been collected because of this duplicate release. After this change, they may leak, potentially even leaving files on disk. However, imo this patch implements the "right" behavior, and we should deal with other issues as they appear. I've done some testing, but it's inconclusive. I ran `./gradlew check` with and without this patch, and in both cases, attribute files were retained in `/tmp`. Can't tell whether this patch introduced new leaks. Addresses #6705 * not PendingFeature
|
Thank you very much @yawkat, it seems to be fixed :) |
|
Ok, so it only resolved the issue with
|
graemerocher
added
the
status: pr submitted
yawkat
removed
the
status: pr submitted
yawkat
added a commit
that referenced
this issue
Jan 17, 2022
`NettyHttpRequest.buildBody` distinguishes between requests with "received data" and "received content", where the former only applies when `AbstractHttpData` instances are present. `AbstractHttpData` is a base class of both `DiskAttribute` and `MemoryAttribute`, but not of `MixedAttribute`. This makes `buildBody` use the "content" path for mixed requests. This change adds an exception for `MixedAttribute` so that it is also covered by the "data" path. Fixes #6705
jameskleeh
pushed a commit
that referenced
this issue
Jan 18, 2022
`NettyHttpRequest.buildBody` distinguishes between requests with "received data" and "received content", where the former only applies when `AbstractHttpData` instances are present. `AbstractHttpData` is a base class of both `DiskAttribute` and `MemoryAttribute`, but not of `MixedAttribute`. This makes `buildBody` use the "content" path for mixed requests. This change adds an exception for `MixedAttribute` so that it is also covered by the "data" path. Fixes #6705
yawkat
added a commit
that referenced
this issue
Jan 21, 2022
`NettyHttpRequest.buildBody` distinguishes between requests with "received data" and "received content", where the former only applies when `AbstractHttpData` instances are present. `AbstractHttpData` is a base class of both `DiskAttribute` and `MemoryAttribute`, but not of `MixedAttribute`. This makes `buildBody` use the "content" path for mixed requests. This change adds an exception for `MixedAttribute` so that it is also covered by the "data" path. Fixes #6705
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected Behavior
No error are present when using
multipart.disk = trueandmultipart.mixed = true.Actual Behaviour
Errors presented below are present.
Steps To Reproduce
There are 2 described issues:
On route
/there is login form for micronaut security login endpoint. You can write anything in those fields and click submit. After that, errorInternal Server Error: refCnt: 0, decrement: 1is thrown. But when you remove configurationmixed: trueanddisk: true, everything is running fine.On route
/uploadthere is multipart form with file selection. Please select filebig.txtcontained in the root of this repository and click submit. Request in browser will hang. But, when you stop the server, theEntity too largeresponse is sent.Environment Information
No response
Example Application
https://github.com/morki/micronaut-multipart-issue
Version
3.2.3
The text was updated successfully, but these errors were encountered: