Handle canceled keys in TCP client by techee · Pull Request #411 · dnsjava/dnsjava

techee · 2026-07-10T19:18:49Z

Disclaimer: the whole patch, including the description below, was generated using Claude Fable using the xhigh effort - I only experienced the crash on Android and asked it to fix the problem, but I didn't dig into the problem myself. Please let me know if the patch needs any adjustments.

Problem

A failed write in NioTcpClient.ChannelState#processWrite (e.g. the server resetting the
connection while multiplexed queries are still being written) kills the shared selector
thread with an unhandled CancelledKeyException:

java.nio.channels.CancelledKeyException
    at sun.nio.ch.SelectionKeyImpl.ensureValid(SelectionKeyImpl.java:73)
    at sun.nio.ch.SelectionKeyImpl.interestOps(SelectionKeyImpl.java:82)
    at org.xbill.DNS.NioTcpClient$ChannelState.processWrite(NioTcpClient.java:282)
    at org.xbill.DNS.NioTcpClient$ChannelState.processReadyKey(NioTcpClient.java:148)
    at org.xbill.DNS.NioClient.processReadyKeys(NioClient.java:255)
    at org.xbill.DNS.NioClient.runSelector(NioClient.java:156)

There are two escape paths:

processWrite catches the IOException from Transaction#send, calls key.cancel(),
and then falls through to key.interestOps(SelectionKey.OP_READ) on the canceled key.
Even with 1 fixed, processReadyKey calls key.isReadable() right after processWrite
returns; on the now-canceled key this also throws (isReadable → readyOps →
ensureValid).

Since runSelector only handles IOException/ClosedSelectorException, the exception
kills the selector thread. All pending and future queries then never complete — query
timeouts run on that same thread — and on Android, where an uncaught exception on any
thread is fatal, the whole application crashes. We see this regularly in the field from an
app whose DNS tool issues parallel queries with DNSSEC (truncation → TCP fallback,
multiplexed on one channel) against home routers that reset the TCP connection mid-write.

This is the TCP counterpart of #129 ("Handle canceled keys in UDP client", fixed in 3.6.5).

Fix

processWrite: treat a write failure like the other error paths in this class
(processConnect, processRead) — a failed write leaves the TCP stream in an undefined
state for every transaction multiplexed on the channel, so fail them all via
handleChannelException, cancel the key, and return without touching interestOps.
processReadyKey: re-check key.isValid() before the isReadable() branch, since
processWrite may have canceled the key.

Test

testWriteFailureDoesNotKillSelectorThread reproduces the crash deterministically: large
queries against a server with a 16-byte receive buffer (the testResponseStream trick)
leave transactions queued mid-write with OP_WRITE armed; the server then resets the
connection, so the next write fails inside processWrite. The test asserts that all
transactions complete exceptionally and that the selector thread survives (watched via an
uncaught-exception handler, the same pattern as
testProcessReadyKeysShouldNotThrowConcurrentModificationException).

Without the fix the test fails deterministically (selector thread dies, transactions never
complete); with the fix, the whole NioTcpClientTest/NioUdpClientTest suites pass.

A failed write in ChannelState#processWrite canceled the selection key and then fell through to key.interestOps(OP_READ), throwing CancelledKeyException. The exception escaped runSelector's IOException handling and killed the shared selector thread: all pending and future queries hung forever (their timeouts run on that thread), and on Android, where an uncaught exception on any thread is fatal, the whole app crashed. Triggered in the field by servers that reset a TCP connection while multiplexed queries are still being written. Treat a write failure like the other error paths in this class: fail all transactions multiplexed on the channel, close it and stop processing the key. Also re-check key validity before the isReadable check in processReadyKey, which would otherwise throw for the canceled key right after processWrite returns. This is the TCP counterpart of dnsjava#129.

codecov · 2026-07-10T19:24:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.79%. Comparing base (06a0599) to head (0ce1ec8).

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #411      +/-   ##
============================================
+ Coverage     66.69%   66.79%   +0.09%     
  Complexity     3074     3074              
============================================
  Files           198      198              
  Lines         13645    13645              
  Branches       2126     2126              
============================================
+ Hits           9101     9114      +13     
+ Misses         3966     3954      -12     
+ Partials        578      577       -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sonarqubecloud · 2026-07-10T19:24:23Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

techee · 2026-07-10T19:28:14Z

This is the text from claude code for more context:

What the PR contains (commit 0ce1ec8, "Handle canceled keys in TCP client")

The fix turned out to need two changes, not one — and the test proved it:

processWrite: a failed write now routes through handleChannelException —
failing every transaction multiplexed on the channel and closing it, matching
how processConnect/processRead already handle errors — then cancels the key
and returns, instead of falling through to interestOps() on the cancelled key.
processReadyKey: a validity re-check before the isReadable() branch. This
one I only discovered because my first test run failed with fix 1 applied:
isReadable() internally calls ensureValid(), so it throws the very same
CancelledKeyException one line after processWrite returns. A one-line fix
would have shipped the same crash through a second door.

Plus a regression test, testWriteFailureDoesNotKillSelectorThread, built from
the repo's own idioms: it fills the client's send buffer against a server with
a 16-byte receive buffer (their testResponseStream trick) so transactions are
stuck mid-write, then RSTs the connection — hitting the write-failure path
deterministically. It asserts the transactions fail cleanly and that the
selector thread survives a grace period, because I caught my first version
false-passing when its assertions raced the thread's death.

Verification, both directions: without the fix the test fails
deterministically (selector thread dies, transactions hang — your production
symptom exactly); with it, the full NioTcpClientTest + NioUdpClientTest suites
pass (12/12), spotless-formatted. I used a self-contained Maven in
~/Downloads/apache-maven-3.9.16 with the Android Studio JDK — no system
changes.

★ Insight ─────────────────────────────────────

The repo's history was the design guide: the maintainer fixed the identical
bug in the UDP client four months ago (java.nio.channels.CancelledKeyException #129, released in 3.6.5) with exactly
this shape — channel-level failure handling, key.cancel() + return, and a
selector-thread-watching test. The TCP side was simply left behind, which
makes this PR an easy "complete the symmetry" review.
The red-test-first discipline paid for itself twice here: it exposed the
second escape path (isReadable → ensureValid) that code inspection missed, and
it exposed the test's own race. A patch verified only by reading would have
been wrong in both ways.
─────────────────────────────────────────────────

techee · 2026-07-10T19:31:45Z

Forgot to mention - feel free to take over the patch and commit it yourself. I'm not its author anyway so no credits needed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Handle canceled keys in TCP client#411

Handle canceled keys in TCP client#411
techee wants to merge 1 commit into
dnsjava:masterfrom
techee:fix-tcp-write-cancelled-key

techee commented Jul 10, 2026

Uh oh!

codecov Bot commented Jul 10, 2026

Uh oh!

sonarqubecloud Bot commented Jul 10, 2026

Uh oh!

techee commented Jul 10, 2026

Uh oh!

techee commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

techee commented Jul 10, 2026

Problem

Fix

Test

Uh oh!

codecov Bot commented Jul 10, 2026

Codecov Report

Uh oh!

sonarqubecloud Bot commented Jul 10, 2026

Quality Gate passed

Uh oh!

techee commented Jul 10, 2026

Uh oh!

techee commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant