Skip to content

Avoid the Netty global executor race in instrumentation tests - #13050

Draft
AlexeyKuznetsov-DD wants to merge 1 commit into
masterfrom
alexeyk/fix-netty-test-shutdown-race
Draft

AlexeyKuznetsov-DD wants to merge 1 commit into
masterfrom
alexeyk/fix-netty-test-shutdown-race

Conversation

@AlexeyKuznetsov-DD

Copy link
Copy Markdown
Contributor

What Does This Do

  • Use Netty 4.0.4.Final instead of 4.0.0.Final for the netty-concurrent-4.0 test baseline, with the corresponding lockfile update.
  • Bound the scheduled-future tests' executor-group shutdown wait to 5 seconds and report a clear assertion failure on timeout or interruption.

Production compilation and muzzle compatibility still start at 4.0.0.Final. Scope and continuation checks remain enabled.

Motivation

An ARM64 Java 27 test run exhausted the 20-minute task deadline while closing an executor group in testDelayedScheduledFutureTaskActivatesCapturedContinuationWhenDelayExpires. The child executor had already terminated; its parent group never received the termination notification.

This matches the upstream GlobalEventExecutor race addressed by Netty commit 28b8573 — Fix a potential race condition where the worker thread of GlobalEventExecutor terminates with a pending task, dated July 18, 2013. 4.0.4.Final is the first 4.0 patch release containing that commit (absent from the 4.0.3.Final tag, present in 4.0.4.Final).

The original submission order checks/starts the worker before enqueueing the task. An existing worker can exit between those operations, leaving the notification queued with nobody to execute it. The upstream change enqueues first, then checks/starts the worker.

sequenceDiagram
    autonumber
    participant C as Child executor
    participant Q as Notification queue
    participant W as Global worker
    participant G as Executor group
    participant T as Test worker

    rect rgb(255, 235, 238)
        Note over C,T: BEFORE · Netty 4.0.0.Final · lost termination notification
        C->>W: startThread() observes an existing worker
        W->>Q: Check for pending tasks
        Q-->>W: Empty
        W->>W: Exit and reset state to NOT_STARTED
        C->>Q: addTask(child termination listener)
        Note over Q,W: Heap: 1 queued task, worker already terminated
        T->>G: Wait for group termination
        Note over G,T: Heap: terminatedChildren = 0 / 1<br/>Group promise remains pending
    end

    rect rgb(232, 245, 233)
        Note over C,T: AFTER · Netty 4.0.4.Final · enqueue before starting
        C->>Q: addTask(child termination listener)
        C->>W: startThread() starts a worker if needed
        W->>Q: Poll pending task
        Q-->>W: Child termination listener
        W->>G: Notify child termination
        G-->>T: Complete group termination promise
        Note over G,T: Cleanup completes<br/>A separate 5-second guard reports future regressions
    end
Loading

Additional Notes

Thread and heap evidence

The matching thread dump showed the test worker waiting in cleanup:

io.netty.util.concurrent.DefaultPromise.await(DefaultPromise.java:240)
io.netty.util.concurrent.DefaultPromise.sync(DefaultPromise.java:204)
executor.NettyScheduledFutureTaskContextPropagationTest
  $CloseableDefaultEventExecutorGroup.close(...)
executor.NettyScheduledFutureTaskContextPropagationTest
  .testDelayedScheduledFutureTaskActivatesCapturedContinuationWhenDelayExpires(...)

The heap captured during the stall identifies the missing notification:

Component Observed state
Child executor TERMINATED
Child termination promise Completed successfully; listeners already detached
Parent executor group terminatedChildren = 0, children.length = 1; termination promise unresolved
Global executor NOT_STARTED; previous worker thread terminated; one task queued
Stranded task PromiseTask → RunnableAdapter → DefaultPromise$3, referencing that child's termination promise and the parent's MultithreadEventExecutorGroup$1 listener

The object references connect the queued callback to the exact child and parent involved in the blocked cleanup. This is a notification-dispatch failure. The evidence does not implicate slow class retransformation or establish an incorrect continuation ownership transition.

Agent-free reproduction

A temporary standalone Java 17 reproducer controlled the interleaving at the enqueue boundary: hold submission immediately before queue insertion, let the previous global worker terminate, then release submission. It loaded Netty without the Datadog agent and asserted the expected result for each version:

Netty Previous worker terminated Notification completed Pending tasks
4.0.0.Final Yes No 1
4.0.4.Final Yes Yes 0

The reproducer uses reflective queue replacement to make the race deterministic and is not included in the repository. The production test change adopts the upstream correction and adds a bounded cleanup assertion. A single successful run against the original version would not exclude this intermittent race.

Validation

The module's allTests task completed on local ARM64 Java 8 and Java 27. It covers test, forkedTest, executorTest, legacyNettyTest, legacyNettyTestForkedTest, netty4144Test, and netty4144TestForkedTest (latest-dependency suites are excluded by the task's normal convention).

Runtime Passed Skipped Failures / errors
ARM64 Java 8 263 22 0
ARM64 Java 27 263 22 0
./gradlew :dd-java-agent:instrumentation:netty:netty-concurrent-4.0:allTests -PtestJvm=8
./gradlew :dd-java-agent:instrumentation:netty:netty-concurrent-4.0:allTests -PtestJvm=27

The affected nine-case scheduled-future test class also passed separately on Java 27. ARM64 CI confirmation is pending.

The change is confined to test dependencies and test cleanup; it adds no tracer runtime work.

Contributor Checklist

@AlexeyKuznetsov-DD AlexeyKuznetsov-DD added type: bug fix Bug fix comp: testing Testing tag: no release notes Changes to exclude from release notes inst: netty Netty instrumentation tag: ai generated Largely based on code generated by an AI or LLM labels Oct 25, 2026
@datadog-prod-us1-3

Copy link
Copy Markdown

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 68.82% (+8.81%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d928437 | Docs | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)

Suite Status
Startup 🟡 warning

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 13.97 s 13.97 s [-0.8%; +0.9%] (no difference)
startup:insecure-bank:tracing:Agent 12.89 s 13.01 s [-1.8%; -0.2%] (maybe better)
startup:petclinic:appsec:Agent 17.88 s 17.60 s [+0.5%; +2.6%] (maybe worse)
startup:petclinic:iast:Agent 17.55 s 17.59 s [-1.1%; +0.7%] (no difference)
startup:petclinic:profiling:Agent 17.48 s 17.29 s [+0.1%; +2.0%] (maybe worse)
startup:petclinic:sca:Agent 17.81 s 17.69 s [-0.2%; +1.5%] (no difference)
startup:petclinic:tracing:Agent 16.68 s 16.75 s [-1.5%; +0.6%] (no difference)

Commit: d928437f · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: testing Testing inst: netty Netty instrumentation tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: bug fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant