Croway commented on code in PR #29103:
URL: https://github.com/apache/camel/pull/29103#discussion_r4456928529
##########
components/camel-netty-http/src/main/java/org/apache/camel/component/netty/http/DefaultNettyHttpBinding.java:
##########
@@ -521,7 +524,14 @@ public HttpResponse toNettyResponse(Message message,
NettyHttpConfiguration conf
}
if (buffer == null) {
// fallback to byte array as last resort
- byte[] data = message.getBody(byte[].class);
+ byte[] data;
+ try {
+ data = message.getBody(byte[].class);
+ } catch (TypeConversionException e) {
Review Comment:
This catch turns every failed `byte[]` conversion into a 200 with the body's
`toString()`, not only the `List` case. In a test with a type converter from
`Thing` to `byte[]` that throws a `TypeConversionException` wrapping
`IOException("disk on fire")`, the PR answers `200` with the body
`Thing.toString()`, where main answers `500`.
#29111 fixes the `List` case at the root: `ArrayTypeConverter` gives no
conversion, so `getBody(byte[].class)` returns `null` and the `String` fallback
below is used. In a tree with this PR and #29111 merged and this catch removed,
`NettyHttpReplyBodyTest` passes 3/3 (the `List` reply is `200 [a, b]`), and the
failing converter gets its 500 back. As a control, without #29111 and without
the catch, `testListReply` fails with `expected: <200> but was: <500>`.
Could the catch be dropped and this PR rebased on #29111 once it is merged?
##########
components/camel-netty-http/src/main/java/org/apache/camel/component/netty/http/DefaultNettyHttpBinding.java:
##########
@@ -511,7 +512,9 @@ public HttpResponse toNettyResponse(Message message,
NettyHttpConfiguration conf
if (response == null) {
if (body instanceof ByteBuf byteBuf) {
- buffer = byteBuf;
+ // the request body (with disableStreamCache) is released when
the exchange is done, so a reply that
+ // sends it back must write a reference of its own, as writing
the response releases the buffer too
+ buffer = isRequestBody(message, byteBuf) ?
byteBuf.retainedDuplicate() : byteBuf;
Review Comment:
The extra reference is taken here, before the response is built. When
building it fails afterwards, the duplicate is never written or released.
In a test with `disableStreamCache=true`, a route echoes the request body
and also sets a header Netty rejects (value `"line one\nline two"`, or name `"X
Bad"`). `headers().add` throws `IllegalArgumentException: a header value
contains prohibited character 0xa at index 8`, and
`HttpServerChannelHandler.getResponseBody` sends a 500. The request buffer then
stays at `refCnt` 1, and the paranoid leak detector reports `LEAK:
ByteBuf.release() was not called before it's garbage-collected`. On main the
same route also gets a 500, but the buffer reaches `refCnt` 0.
Releasing `response` (which holds the duplicate) when the rest of
`toNettyResponse` throws, or taking the retained duplicate only once the
headers are set, would avoid it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
