minor: migrate Druid from Netty 3.10 to Netty 4.1 - #19754
Open
sklochkov2 wants to merge 33 commits into
Open
Conversation
- Pin netty4.version to 4.1.135.Final and drop the netty3.version property - Replace the io.netty:netty (v3) artifacts with explicit netty4 modules (netty-common/buffer/codec-http/handler/transport) per module - Remove obsolete direct netty3 deps (quidem-ut, ranger-security, rabbit-stream-indexing-service, kafka-indexing-service); keep defensive io.netty:netty exclusions so transitive netty3 stays off the classpath Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate the Netty HTTP client stack to io.netty 4.1: NettyHttpClient, HttpClientInit/Config, Request, pool/ChannelResourceFactory, the pipeline factory, all response/* handlers and holders, and FrameFileHttpResponseHandler. ChannelBuffer->ByteBuf, HttpChunk->HttpContent, event-model and status-API updates; content is now delivered via handleChunk. Preserves latest-only features (e.g. TrafficCop.abort()). Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate the RPC framework (RequestBuilder/ServiceClientImpl/retry policies/ OverlordClientImpl/IgnoreHttpResponseHandler), clients (DirectDruidClient, CoordinatorClientImpl, DataServerClient, BrokerClientImpl, MessageRelayClientImpl), coordinator loading/sync (HttpLoadQueuePeon, ChangeRequestHttpSyncer, LookupCoordinatorManager) and lookups to io.netty 4.1. Status/header API updates and HttpResponseStatus.code()/reasonPhrase(). Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate RemoteTaskActionClient, HttpShuffleClient, ParallelIndex* clients, WorkerTaskRunnerQueryAdapter, hrtr/HttpRemoteTaskRunner + WorkerHolder, SeekableStreamIndexTaskClientAsyncImpl, WorkerTaskManager, TaskRunnerUtils and OverlordCompactionResource to io.netty 4.1 (status API + content handling). Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate sql SystemSchema/SystemServerPropertiesTable and services CoordinatorRuleManager to io.netty 4.1 (status/content API; read body from StringFullResponseHolder). Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate multi-stage-query (Dart clients, BaseWorkerClientImpl, IndexerController client + SketchResponseHandler, SqlStatementResource, retry policies) and extensions (druid-basic-security, druid-catalog, druid-kerberos incl. RetryIfUnauthorizedResponseHandler) to io.netty 4.1. Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate all unit tests and shared test infrastructure to io.netty 4.1. Update TestHttpClient and MockServiceClient to mimic the real Netty pipeline (headers via handleResponse, body via handleChunk) so content reaches handlers without double-counting; populate response holders where built directly; migrate DefaultHttpResponse.setContent()/getContent() overrides to DefaultFullHttpResponse/content(); freshly migrate the embedded-tests module. Co-authored-by: Cursor <cursoragent@cursor.com>
Pinning Netty to 4.1.x makes the managed version lower than the 4.2.7 that the test-scoped docker-java-transport-netty:3.7.0 requests, which trips the maven-enforcer RequireUpperBoundDeps rule at the validate phase and aborts every CI build. Exclude the five Netty artifacts docker-java declares from that rule (the netty-bom already converges them to a single version); enforcer 3.6.2 ignores groupId-only excludes, so each is listed as groupId:artifactId. Also bump netty4.version 4.1.135 -> 4.1.136 (no incompatible changes) and reconcile the licenses.yaml Netty entry to 4.1.136, dropping the 4.2-only codec-split modules that do not exist on the 4.1 line. Co-authored-by: Cursor <cursoragent@cursor.com>
…nse check Addresses the second batch of CI failures on the Netty 4.1 branch. JDK 25 forked-VM crash (AsyncHttpClientTest): - The mock server's accept loop ran on a non-daemon thread and busy-spun at 100% CPU once the ServerSocket was closed (Thread.sleep() could swallow the interrupt, so accept() on a closed socket threw immediately in a tight loop). Under the CI JFR agent this flooded allocations and killed the VM. - Fix: run the accept loop on a daemon thread and also exit when the socket is closed (!serverSocket.isClosed()). JDK 25 read-timeout hang (JankyServersTest, [T*,F*,G*,J*] shard timeout): - Netty 4's ReadTimeoutHandler schedules on the NioEventLoop, whose select()/ epoll_wait can be interrupted by signals (e.g. the profiler agent), delaying or dropping scheduled timeouts on JDK 25. Read timeouts are now driven off the shared HashedWheelTimer (via a new TimerReadTimeoutHandler), restoring the Netty-3-era timer-thread semantics that are immune to the event-loop issue. - While fixing this, found that Netty's public ReadTimeoutException(String) constructor trips `assert shared` (ChannelException) and throws AssertionError when assertions are enabled (surefire runs with -ea). That caused setException() to throw and left the response future uncompleted, hanging get(). We now raise the exception via the no-arg ReadTimeoutException() constructor. Resource-leak hardening (SequenceInputStreamResponseHandler): - Release retained ByteBuf/close streams if the queue put is interrupted, so an interrupt during chunk delivery can't leak direct memory. Deprecated Netty 4 API cleanup: - Replace HttpResponse.getStatus() with status() in StandardRetryPolicy, BytesAccumulatingResponseHandler, and LookupCoordinatorManager. - Wrap ByteBufInputStream usage in try-with-resources in RequestBuilderTest. License check: - Bump netty-tcnative to 2.0.78.Final in licenses.yaml to match the version pulled in by the Netty 4.1.136 BOM. Co-authored-by: Cursor <cursoragent@cursor.com>
…eclare deps Third batch of CI fixes on the Netty 4.1 branch. Malformed-response handling (JankyServersTest.testHttpEchoServer): - Netty 3's HTTP decoder threw during decoding of a malformed response, which propagated to exceptionCaught and failed the request future. Netty 4's decoder instead emits a message flagged with a failed DecoderResult and never throws, so the malformed response was processed as if valid and the caller only saw a later "Channel disconnected". NettyHttpClient now inspects decoderResult() on inbound HttpObjects and surfaces the underlying cause (e.g. "invalid version format: GET"), restoring the pre-Netty-4 behavior. Deprecated-API check (HttpClientConfig): - The worker-count default touched the deprecated JvmUtils.getRuntimeInfo() (the fork's thread-cap change modified that line, so the incremental deprecation check flagged it). Compute the default from Runtime.getRuntime() .availableProcessors() instead; the MAX_WORKER_THREADS cap is preserved. Dependency analysis (druid-server): - dependency:analyze reported netty-buffer, netty-codec-http, and netty-transport as used-but-undeclared. Declare them explicitly (versions come from netty-bom). Co-authored-by: Cursor <cursoragent@cursor.com>
Fourth batch of CI fixes on the Netty 4.1 branch. Dependency analysis (druid-processing): - dependency:analyze reported io.netty:netty-codec as used-but-undeclared (it is pulled in directly, including by the DecoderResult handling added in the previous commit). Declare it explicitly; the version comes from netty-bom. Diff branch-coverage: - The branch's diff coverage fell just under the required 50% because the new HashedWheelTimer-based read-timeout handler (TimerReadTimeoutHandler) was only exercised along its "fire once" path by the existing socket-based tests; its reschedule-on-read, channelActive, channelRead, handler-added-while-inactive, and teardown paths were untested. - Add TimerReadTimeoutHandlerTest, which drives the handler through an EmbeddedChannel and a real HashedWheelTimer to cover those branches deterministically. To enable this, TimerReadTimeoutHandler is made package-private (no behavior change). Co-authored-by: Cursor <cursoragent@cursor.com>
…analyze
Netty 4 splits the former single io.netty:netty jar into per-feature
artifacts, and dependency:analyze (run with -DfailOnWarning=true) requires
each module to explicitly declare every Netty artifact it references at the
bytecode level, not just the ones it imports. For example, calling
HttpResponse#content() pulls in netty-buffer even without an io.netty.buffer
import.
Rather than fix these one CI round-trip at a time, a full-reactor
dependency:analyze pass (all 78 modules, without failOnWarning) was used to
surface every missing declaration at once. This adds the exact artifacts each
module was using transitively:
- indexing-service: netty-buffer, netty-codec-http
- sql: netty-codec-http
- services: netty-codec-http
- druid-kerberos: netty-buffer
- kubernetes-overlord-extensions: netty-codec-http
Versions are inherited from the managed Netty BOM in core modules and follow
the existing explicit ${netty4.version}/provided pattern in the extensions.
No module had an unused Netty declaration, so nothing was removed.
Co-authored-by: Cursor <cursoragent@cursor.com>
Resolves a single conflict in StreamIndexFaultToleranceTest: an upstream commit added a test that imports HttpMethod (as the Netty 3 org.jboss.netty.handler.codec.http.HttpMethod) plus org.hamcrest.Matchers, while this branch had already migrated the file's import to the Netty 4 io.netty.handler.codec.http.HttpMethod. Kept the Netty 4 import and the new Matchers import, dropped the reintroduced Netty 3 import. Verified the merged tree contains no remaining org.jboss.netty references in sources or Netty 3 declarations in poms, and that the root pom still pins netty4.version 4.1.136.Final with the enforcer exclusions intact. Co-authored-by: Cursor <cursoragent@cursor.com>
…hang
FriendlyServersTest repeatedly consumed the unit-test job's entire 60 minute
budget rather than failing. The hang reproduces locally and deterministically
when AsyncHttpClientTest runs before FriendlyServersTest in the same fork; a
thread dump of the stuck JVM shows the main thread parked in
NettyHttpClient.go(NettyHttpClient.java:142) <- awaitUninterruptibly()
FriendlyServersTest.testFriendlyProxyHttpServer(...:171)
waiting on the proxy CONNECT promise, while all eight Netty worker threads sit
idle in EPoll.wait having used ~16ms of CPU over 83s. There is no pending I/O,
so this is not the event-loop wakeup problem that the read-timeout work in this
branch dealt with; it is an unbounded wait.
Two defects combine to produce it.
The product defect is in ChannelResourceFactory: the proxy CONNECT exchange is
an ordinary application-level request/response and nothing bounds it. The SSL
path immediately below it calls setHandshakeTimeoutMillis, but the CONNECT path
has no equivalent, and ChannelOption.CONNECT_TIMEOUT_MILLIS only covers the TCP
connect, which succeeds. A proxy that accepts the connection but never replies
therefore leaves proxyConnectPromise uncompleted forever, and because
NettyHttpClient#go waits on it uninterruptibly the caller can never recover.
This is a production hazard too, not just a test artifact: any Druid process
talking through a wedged HTTP proxy could park a thread permanently.
The test defect is that the mock proxy called Assert.fail() from a background
thread. The AssertionError it throws is not an Exception, so it escaped the
accept loop and died inside a Future nobody inspects, silently stopping the
server. The ServerSocket stayed open, so the client's connect still succeeded
into the listen backlog and simply never received an answer.
Fixes:
- ChannelResourceFactory bounds the CONNECT handshake at 10s, mirroring the
adjacent SSL handshake timeout. The deadline is tracked on the shared
HashedWheelTimer rather than the channel's event loop, consistent with the
read-timeout handling earlier in this branch, and is cancelled as soon as
the promise completes.
- The mock proxy records failures instead of calling Assert.fail, keeps
serving (a pooled client may legitimately open and drop connections), and
its readLine loops now handle EOF instead of spinning or throwing NPE. A
recorded server-side error is attached as a suppressed exception when the
request fails, so the informative error is not lost.
- JankyServersTest gains testSilentProxyServer, pointing the client at the
existing silent server, and the proxy test gets @test(timeout = 60_000L) so
a regression fails the test instead of the whole CI job.
Attribution was verified rather than assumed. With the original test but the new
timeout, the test fails in 10.1s with "Timed out after [10,000] ms waiting for a
CONNECT response from proxy[anotherHost:8080]", confirming both that the timeout
works and that the server was dying before answering CONNECT. With both changes
the previously always-hanging combination passed 10/10 runs, and the full
org.apache.druid.java.util.http.client package is 49/49 green on JDK 25 with no
checkstyle or forbidden-apis violations.
Co-authored-by: Cursor <cursoragent@cursor.com>
java.net.URL's string constructor has been deprecated since Java 20, and this branch now compiles at release 25, so the two call sites added by the previous commit produce deprecation warnings. Replace them with URI.create(...).toURL(), which is the documented replacement and performs the same RFC 2396 parsing. Only the lines introduced by the previous commit are changed. The surrounding tests still use new URL(...) in places this branch never touched, so URL remains imported; converting those is left to a separate cleanup rather than mixing unrelated churn into the CONNECT-timeout fix. Co-authored-by: Cursor <cursoragent@cursor.com>
Now that the loop conditions null-check the line first, the null-safe "".equals(line) idiom buys nothing, so check the string directly. Prefer isEmpty() over length() != 0: it states the intent, and it is what the rest of the codebase uses (303 files under processing/src use isEmpty(), none use length() != 0). Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate the Druid codebase to Netty 4.1
github-actions
Bot
added
Area - Batch Ingestion
Area - Querying
Area - Streaming Ingestion
Area - Dependencies
Kubernetes
Area - Ingestion
Area - MSQ
FrankChen021
left a comment
FrankChen021
left a comment
Member
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 1 |
| P3 | 0 |
| Total | 3 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 1 |
| P3 | 0 |
| Total | 3 |
Reviewed 167 of 167 changed files.
This is an automated review by Codex GPT-5.6-Sol
Findings that could not be attached inline:
- processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java:327 - [P2] Release inbound messages when handlers throw. Normal branches release
msg, but handler, parsing, or limit exceptions jump here first.ChannelInboundHandlerAdapterdoes not auto-release, so exceptional responses leak their pooled buffer. Release the inbound message in afinallyblock.
| { | ||
| final int length = buffer.readableBytes(); | ||
| return new InputStreamHolder(new ChannelBufferInputStream(buffer), chunkNum, length); | ||
| buffer.retain(); |
Member
There was a problem hiding this comment.
[P1] Release retained chunks when abandoning streams
fromByteBuf retains every buffer, but query close/error paths discard queued holders with queue.clear() without closing them. Repeated cancelled or failed queries can therefore exhaust pooled direct memory. Drain and close queued and current streams before clearing them.
Author
There was a problem hiding this comment.
Addressed this in a subsequent commit.
| method, | ||
| urlFile.isEmpty() ? "/" : urlFile | ||
| urlFile.isEmpty() ? "/" : urlFile, | ||
| request.hasContent() ? request.getContent() : Unpooled.EMPTY_BUFFER |
Member
There was a problem hiding this comment.
[P1] Preserve request bodies for Kerberos retries
DefaultFullHttpRequest takes ownership of this buffer and Netty releases it after encoding. A subsequent Kerberos 401 retry calls request.copy() on the released buffer, causing IllegalReferenceCountException for requests with bodies. Use independent outbound content or non-reference-counted body storage.
Author
There was a problem hiding this comment.
Addressed this in a subsequent commit.
Member
There was a problem hiding this comment.
Thanks—the retry itself now preserves the bytes, but buffer ownership is still unresolved. retainedDuplicate() lets Netty release only its duplicate, leaving Request.content at refCnt == 1 after every send. Request has no close/release lifecycle, and Request.copy() creates another independently reference-counted buffer for each Kerberos retry, so pooled/direct request bodies can remain allocated indefinitely. This also contradicts the existing Request#setContent(byte[]) comment that the body is released after the write. Please store the reusable body in non-reference-counted form and create an outbound buffer per attempt, or add an explicit lifecycle that releases the original and retry copies after the final attempt.
Reviewed 167 of 167 files in the supplied full diff.
Author
There was a problem hiding this comment.
That's a good find, pushed a fix.
Member
|
there's already a PR doing the same thing: #19567 |
Author
Yes, I saw that. Decided to post this patchset for review anyway because it has been running on production druid clusters for a while (albeit being applied on top of version 31.0.2) so the risk may be somewhat lowered, what do you think? |
NettyHttpClient handed request.getContent() straight to DefaultFullHttpRequest. That gives Netty ownership of the caller's buffer: HttpObjectEncoder extends MessageToMessageEncoder, which releases whatever it encodes, and the socket write advances the buffer's reader index. After a single send the caller was therefore left holding a released, fully consumed buffer. Callers do reuse a Request. KerberosHttpClient resends request.copy() after a 401, and ClientUtils copies a request's content to retarget it at another server; both call ByteBuf.copy(), which throws IllegalReferenceCountException once refCnt has reached zero. Even if it had not thrown, the consumed reader index means the retry would have sent an empty body. Pass a retainedDuplicate() instead. It shares the bytes but carries its own reader/writer indices, so Netty's release balances our retain and the caller's buffer is left untouched and resendable. The new FriendlyServersTest case fails without this change (refCnt 0 rather than 1 after sending) and covers the retry itself, asserting that a resent request.copy() still carries its body. Co-authored-by: Cursor <cursoragent@cursor.com>
DirectDruidClient buffers response chunks as InputStreamHolders, and
InputStreamHolder.fromByteBuf retains the Netty buffer it wraps, handing it to a
ByteBufInputStream that releases it only when closed. That retain is what keeps
the bytes alive past NettyHttpClient, which releases each inbound HttpContent as
soon as handleChunk returns.
Both teardown paths discarded those holders with queue.clear(), which drops the
references without closing them, so the retained buffers were never released.
Every cancelled or failed query leaked its buffered chunks, and since these come
from socket reads they are pooled direct memory. Under Netty 3 the equivalent
buffers were GC'd heap buffers, so clearing was harmless; refcounting makes it a
leak. The pre-existing comment claiming the buffers "can be released" here was
describing the intent, not the behavior.
Drain and close instead, in three places:
- The early-close path now closes each queued chunk. It also drains when done
is already set, instead of returning first: a response that finished but was
never fully read left its remaining chunks buffered.
- The current chunk (already handed to SequenceInputStream, which only closes
a stream when it advances to the next one) is closed too. This is done only
from close(), on the consumer's own thread, since another thread may still
be reading it. Double-closing is safe: ByteBufInputStream.close() is
idempotent in Netty 4.1.
- enqueue() re-checks discard after queueing, closing the race where close()
drains just after a chunk passed the initial check.
The failure path only drains the queue; it must not close the current chunk,
because the consumer may still be reading it and will reach the error holder
next.
The new test drives the response handler directly and fails without this change
(refCnt 2 rather than 1 after abandoning an unread chunk).
Co-authored-by: Cursor <cursoragent@cursor.com>
Address AI reviewer findings
channelRead released msg at the end of each branch, so any exception thrown while processing it skipped straight to the catch block with the message still unreleased. ChannelInboundHandlerAdapter does not release inbound messages for us (unlike SimpleChannelInboundHandler), so the pooled buffer leaked. This is reachable from ordinary operation rather than only from corrupt input: handleResponse and handleChunk are where DirectDruidClient enforces its limits, so a query exceeding maxScatterGatherBytes or hitting its timeout leaks the buffer that tripped it. Move the release into a finally block and drop the per-branch calls, leaving exactly one release that covers the early returns, the unknown-message-type throw, and anything a handler throws. Handlers are already expected to copy or retain whatever they still need, which is what the previous per-branch releases assumed too, so this does not change the ownership contract. Also adds a test for a handler that throws mid-response, which was previously uncovered, asserting the request settles instead of hanging. Co-authored-by: Cursor <cursoragent@cursor.com>
Release the inbound message even when a response handler throws
Author
Addressed this one as well. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| pipeline.addLast("inflater", new HttpContentDecompressor()); | ||
|
|
||
| return pipeline; | ||
| pipeline.addLast("inflater", new HttpContentDecompressor(false)); |
Author
There was a problem hiding this comment.
Addressed this in a subsequent commit.
| asyncRequest, | ||
| holder -> { | ||
| if (holder.getResponse().getStatus().getCode() == HttpStatus.NO_CONTENT_204) { | ||
| if (holder.getResponse().getStatus().code() == HttpStatus.NO_CONTENT_204) { |
Author
There was a problem hiding this comment.
Addressed this in a subsequent commit.
Two kinds of deprecation warning, both introduced by the move to Netty 4 rather than by anything that was already wrong. HttpResponse.getStatus() is the Netty 3 spelling; Netty 4 deprecated it in favour of status(). Most call sites were converted along with the rest of their files, but five were missed because swapping the org.jboss.netty import for io.netty leaves the call itself compiling unchanged. Two are in files this branch otherwise rewrote (MessageRelayClientImpl, DataServerResponseHandler); in SpecificTaskRetryPolicy the import swap is the only change to the file, which is why a line-level review does not flag them. HttpContentDecompressor deprecates both its no-argument and its boolean constructor in 4.1.136, keeping the (int) and (boolean, int) overloads. The added argument is maxAllocation, a cap on the decompression buffer, and the deprecated constructor delegates to this(strict, 0), so passing 0 explicitly preserves the uncapped behaviour exactly. A bare 0 reads as arbitrary, so the call carries a comment saying what it means. Neither change affects behaviour and no tests are added; the existing coverage for these paths (MessageRelayClientImplTest, DataServerClientTest, SpecificTaskServiceLocatorTest, and the http client suites that build this pipeline) passes unchanged. This clears every Netty deprecation in the code this branch touches. One remains in HttpEmitterModule, whose JdkSslContext constructor is deprecated in 4.2 as well, so pinning 4.1 did not cause it; that file is untouched here and left alone. Co-authored-by: Cursor <cursoragent@cursor.com>
FrankChen021
left a comment
FrankChen021
left a comment
Member
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 0 |
| P3 | 0 |
| Total | 2 |
The queued-stream cleanup in DirectDruidClient fixes the first follow-up. The Kerberos retry now preserves bytes but still needs an ownership fix, so I replied inline. Two additional P1 regressions remain in retained data-server chunk cleanup and chunked lookup-response accumulation.
Reviewed 167 of 167 files in the supplied full diff.
This is an automated review by Codex GPT-5.6-Sol
| // Increment queuedByteCount before queueing the object, so queuedByteCount is at least as high as | ||
| // the actual number of queued bytes at any particular time. | ||
| final InputStreamHolder holder = InputStreamHolder.fromChannelBuffer(buffer, chunkNum); | ||
| final InputStreamHolder holder = InputStreamHolder.fromByteBuf(buffer, chunkNum); |
Member
There was a problem hiding this comment.
[P1] Close retained data-server chunks before clearing the queue
InputStreamHolder.fromByteBuf now retains every inbound buffer, but setupResponseReadFailure still discards queued holders with queue.clear(). Query timeouts and transport failures therefore drop the only objects capable of closing those streams after NettyHttpClient releases its own inbound references, leaking pooled direct memory. Drain and close every queued holder here, as the updated DirectDruidClient now does.
Author
There was a problem hiding this comment.
Addressed this in a subsequent commit.
| return null; | ||
| } | ||
| BytesFullResponseHolder holder = delegateResponse.getObj(); | ||
| InputStream stream = holder == null ? null : new ByteArrayInputStream(holder.getContent()); |
Member
There was a problem hiding this comment.
[P1] Avoid copying the accumulated lookup response on every chunk
toInputStream is called after every handleChunk, and holder.getContent() concatenates every chunk received so far into a new byte array each time. A response split into n chunks therefore performs quadratic copying and temporary allocation; large lookup-state responses can cause extreme GC pressure or OOM. Preserve the previous streaming handler, or construct the combined byte array only once from done().
Author
There was a problem hiding this comment.
Addressed this in a subsequent commit.
Resolves one conflict in the root pom, where four version properties sit on adjacent lines and three unrelated changes landed on them. Each side keeps what it owns: this branch deletes netty3.version and pins netty4.version to 4.1.136.Final, upstream bumps postgresql to 42.7.12 and protobuf to 4.33.0. Nothing else in the range touched Netty, and no org.jboss.netty usage was reintroduced. The merge does require widening the enforcer exclusion, though, because it brings async-http-client 3.0.2 -> 3.0.10. The older release built against Netty 4.1.119, so our 4.1.136 pin was an upgrade and only a few test-scoped artifacts tripped RequireUpperBoundDeps. 3.0.10 builds against 4.2.13, so the pin is now a downgrade for every artifact async-http-client touches, and seven more started failing the check. That is worth more than a suppression, since async-http-client is production code: HttpEmitterModule builds a DefaultAsyncHttpClient for the HTTP emitter. I checked that 3.0.10 still links against 4.1.136 by constructing the client the way HttpEmitterModule does, including the JdkSslContext branch, and driving a GET and a POST through it against a local server. Both complete normally, so the pin is safe and the check is only bookkeeping. The exclusion now names every io.netty artifact in the dependency graph rather than just the ones failing today. The rule rejects wildcards (io.netty:* matches nothing, silently un-excluding what the explicit list covered), and the netty-bom already converges the whole group to one version, so the check has nothing left to catch here. Listing the group in full keeps the next upstream bump of a Netty-based dependency from reopening this. The block goes away with the 4.2 upgrade. Co-authored-by: Cursor <cursoragent@cursor.com>
Same leak as the one fixed in DirectDruidClient, in the other handler that buffers InputStreamHolders. fromByteBuf retains the Netty buffer and hands it to a ByteBufInputStream that releases it only on close, so setupResponseReadFailure dropping the holders with queue.clear() left those retains stranded. Every query that timed out or hit a transport error leaked its buffered chunks, and since they come from socket reads that is pooled direct memory. Drain and close instead. Only the queued chunks are closed, never the one the consumer already dequeued, because that one may still be being read on another thread. This is the same split DirectDruidClient's failure path uses. The new tests cover both routes into the teardown, exceptionCaught and an elapsed query timeout; both fail without this change with the chunk still at refCnt 2. They also cover the FullHttpResponse case in handleResponse, where the body arrives with the headers rather than in a later chunk. Co-authored-by: Cursor <cursoragent@cursor.com>
This migration had replaced LookupCoordinatorManager's streaming response handler with one that wraps BytesFullResponseHandler and rebuilds an InputStream after each chunk. BytesFullResponseHolder.getContent() concatenates every chunk received so far into a fresh array, so an n-chunk response copied its accumulated body n times and produced n throwaway arrays. Lookup state responses grow with the number of lookups, so this is exactly the response that should not be copied quadratically. Restore the original SequenceInputStreamResponseHandler, which streams chunks through untouched. That also puts the file back to a plain Netty 3 -> 4 translation of upstream's code. Doing so requires fixing that handler first, which is why the two changes are together. SequenceInputStream's constructor eagerly pulls the first element of its Enumeration, and this Enumeration's nextElement() blocks on queue.take(). Under Netty 3 the queue was never empty at that point, because getContent() always returned a buffer, empty or not. Netty 4 splits headers from body, so a plain HttpResponse carries no content, nothing was queued, and handleResponse blocked forever on the Netty I/O thread that was supposed to deliver the chunks that would unblock it. Queue an empty stream in that case, restoring the invariant the Netty 3 code relied on. That deadlock is why simply reverting to the streaming handler was not enough, and likely why it was swapped out during the migration rather than translated. LookupCoordinatorManager is its only production caller, so nothing else was affected in the meantime. The existing tests here all stub out makeResponseHandler, which is how the real one went uncovered. The added test drives it: it asserts the status is recorded, that the handler still streams rather than buffers, and that a chunked body comes back intact. It hangs against the unfixed handler, so it carries a timeout rather than stalling CI. Co-authored-by: Cursor <cursoragent@cursor.com>
The diff coverage check failed at 33% branch coverage, all of it from this class. Swapping the deprecated HttpResponse.getStatus() for status() touched the two lines of isTaskMismatch, which pulled four already-uncovered branches into the diff; the class had no test at all. The behaviour is worth pinning down regardless: a 400 or 404 carrying another task's id means this task moved and something else took its port, so the request should be retried rather than surfaced as a failure. The test covers both status codes, our own id (including the URL-encoded form ids are actually sent in), a missing id header, an unrelated status, and that the base policy still decides everything else. Takes the two changed lines to full line and branch coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
This lists group IDs that extension authors should mark provided to avoid colliding with the versions Druid bundles. Druid no longer ships org.jboss.netty after this migration, so that entry is stale and io.netty is what now needs the provided scope. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the Netty 4 APIs this migration left deprecated
Request has no close or release step, yet since this migration it held a ByteBuf, so nothing in the class could say who owned the body. Sending handed that buffer to Netty, whose HttpObjectEncoder releases whatever it encodes, which broke the Kerberos 401 retry. The earlier fix for that passed a retained duplicate instead, leaving the caller's buffer at refCnt 1 after every send with nothing able to release it. Neither arrangement is a lifecycle. Before the migration the body was a ChannelBuffer from HeapChannelBufferFactory. ChannelBuffer has no reference counting at all, so there was nothing to own and no lifecycle to get wrong. Restore that: Request holds a byte[], and NettyHttpClient wraps a fresh outbound buffer around those bytes for each attempt, which Netty is then free to release. copy() can share the array, since nothing writes to a body in place; KerberosHttpClient only rewrites the Cookie header before retrying, and ClientUtils only changes the URL. Retries therefore stop duplicating the body as well. To be clear about the blast radius: nothing was leaking. Every caller already supplied a byte[], which setContent wrapped with Unpooled.wrappedBuffer, so the bodies were unpooled heap buffers that release nothing and are reclaimed by GC. No pooled or direct memory was ever reachable through a Request. Dropping the two ByteBuf setContent overloads, whose only caller was ClientUtils, means none can be from now on either. getContent() returning byte[] is not an extra break for extensions: it returned org.jboss.netty.buffer.ChannelBuffer before this migration, so the signature was already changing. The retry test asserted on refCnt and readableBytes, which no longer describe anything; it now compares the body bytes after sending, which is the invariant it was always about. Co-authored-by: Cursor <cursoragent@cursor.com>
Hold request bodies as plain bytes rather than a Netty buffer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Even though AI was used extensively to create this PR, please do not dismiss it as another case of AI slop. The patches have been battle-tested.
The netty 3.10 -> 4.1 migration code has been running in production for about 7 months - albeit as part of an earlier Druid version (31.0.2). The migration was performed as part of meeting the mandatory CVE remediation deadlines.
This PR contains the same tested patchset, adapted for druid-39-CURRENT.
The aim is zero behaviour changes compared to netty 3 code.
Co-Authored-By: Claude 4.5 (original patchset), Claude 4.8 (adapting it to druid-39-CURRENT).
Key changed/added classes in this PR
Core HTTP client (
processing):NettyHttpClient— ported to the Netty 4 inbound handler model (channelRead/HttpContentrather than
messageReceived/HttpChunk); also surfaces failedDecoderResults as exceptions,which Netty 4 reports on the message instead of throwing.
NettyHttpClient.TimerReadTimeoutHandler(added) — replaces Netty'sReadTimeoutHandler,scheduling read timeouts on the shared
HashedWheelTimerinstead of the channel's event loop.ChannelResourceFactory— Netty 4Bootstrap-based pooled connection factory; the proxyCONNECThandshake is now explicitly bounded (it previously had no timeout and could blockcallers of
NettyHttpClient#goindefinitely).HttpClientInit— builds aBootstrap+NioEventLoopGroupin place of Netty 3'sNioClientBossPool/NioWorkerPool.HttpClientPipelineFactory— now extendsChannelInitializer<SocketChannel>instead ofimplementing
ChannelPipelineFactory.HttpResponseHandler— breaking API change:handleChunknow takesio.netty.handler.codec.http.HttpContentinstead oforg.jboss.netty...HttpChunk. Extensionsimplementing this interface must be updated.
Request—ChannelBuffer/HeapChannelBufferFactorytoByteBuf/Unpooled, and Netty'sBase64tojava.util.Base64.SequenceInputStreamResponseHandler—ByteBuflifecycle handling, so retained buffers arereleased rather than leaked when the consumer is interrupted.
HttpClientConfig,StatusResponseHandler,BytesFullResponseHandler,InputStreamResponseHandler,InputStreamFullResponseHandler,StringFullResponseHandler,FrameFileHttpResponseHandlerServer / RPC:
DirectDruidClient,DataServerResponseHandler,ServiceClientImpl,StandardRetryPolicy,LookupCoordinatorManager,HttpLoadQueuePeon,IgnoreHttpResponseHandlerWorkerHolder(indexing-service),SketchResponseHandler(multi-stage-query)Build / dependencies:
pom.xml(Netty 4.1 BOM, drops thenetty3.versionproperty and theio.netty:nettyartifact, adds
requireUpperBoundDepsexclusions), 15 module poms declaring the per-featureNetty 4 artifacts they use, and
licenses.yaml.Tests:
TimerReadTimeoutHandlerTest(added) —EmbeddedChannel-based coverage of the read-timeouthandler's scheduling, rescheduling, and teardown paths.
JankyServersTest— addstestSilentProxyServer, covering a proxy that accepts but neveranswers
CONNECT.FriendlyServersTest— mock proxy no longer self-destructs by callingAssert.failoff-thread.The remaining main-source files are mechanical API renames (
getStatus()tostatus(),HttpHeaders.Names.*toHttpHeaderNames.*,ChannelBuffertoByteBuf) with no behavior change.This PR has: