gianm commented on code in PR #19567:
URL: https://github.com/apache/druid/pull/19567#discussion_r3733819824
##########
embedded-tests/pom.xml:
##########
@@ -912,6 +912,14 @@
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<excludedGroups>docker-test,perf</excludedGroups>
+ <!-- Enable Netty 4 PARANOID leak detection in the embedded tests
because this is where
+ real cross-node HTTP traffic exercises the migrated HttpClient
code paths (chunked
+ responses, long-poll syncs, TLS, ByteBuf-bearing handlers). The
Stage 1 CI workers
+ that happen to run embedded-tests classes pay a small overhead
per class; in exchange
+ we catch ref-count regressions immediately rather than as a
flaky off-heap leak. -->
+ <systemPropertyVariables>
+
<io.netty.leakDetection.level>PARANOID</io.netty.leakDetection.level>
Review Comment:
I think that `PARANOID` with the builtin leak detector logs but doesn't
throw an exception, which probably won't be noticed. Maybe there's some way to
install a custom detector.
##########
processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java:
##########
@@ -254,37 +254,46 @@ public void abort()
assert currentChunkNum == 0;
possiblySuspendReads(response);
+ // Servers (notably Druid's QueryResource on the historical)
often flush the response
+ // status line + headers in one TCP write and the chunked body
in subsequent writes.
+ // Netty 4's AUTO_READ chains reads automatically after a
complete read cycle, but in
+ // practice the body chunks for these split writes are not
picked up without an explicit
+ // ctx.read(), leaving the caller blocked on an InputStream
that never gets bytes.
+ // Force a read here to drain the body chunks for these
multi-write responses.
+ ctx.read();
Review Comment:
How was the stuff in the comment determined? Is there a test that would fail
without this line?
Also: do we really want to do the `ctx.read()` if the `possiblySuspendReads`
call above decided to suspend reads? Seems like no.
Also: the PR description has the related claim that "the root problem with
prior attempts that resulted in the observed hanging is that Netty 4's
`setAutoRead(true)` is just a config flag". It doesn't seem true though. Netty
4's `DefaultChannelConfig#setAutoRead` has some logic that looks like:
```
if (autoRead && !oldAutoRead) {
channel.read();
}
```
I'm asking this stuff because I am hoping to better understand why this PR
isn't succumbing to the hanging problems that plagued the earlier ones.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
