Skip to content

CAMEL-23495: Replace Thread.sleep() with Awaitility in camel-core and camel-management tests#23180

Closed
gnodet wants to merge 4 commits into
apache:mainfrom
gnodet:CAMEL-23495-replace-thread-sleep-with-awaitility
Closed

CAMEL-23495: Replace Thread.sleep() with Awaitility in camel-core and camel-management tests#23180
gnodet wants to merge 4 commits into
apache:mainfrom
gnodet:CAMEL-23495-replace-thread-sleep-with-awaitility

Conversation

@gnodet
Copy link
Copy Markdown
Contributor

@gnodet gnodet commented May 13, 2026

CAMEL-23495

Replace Thread.sleep() synchronization calls with Awaitility in 15 test files across camel-core and camel-management to reduce flakiness under CI load.

Changes by category

Suspend/resume tests (4 files)

  • RouteSedaSuspendResumeTest, DefaultCamelContextSuspendResumeRouteTest, TwoRouteSuspendResumeTest, SedaConsumerSuspendResumeTest
  • Replaced Thread.sleep(1000L) + existing await() with a single await().pollDelay(1s).atMost(5s) to give SEDA consumer thread time to idle after route suspension

Management tests (2 files)

  • ManagedInflightStatisticsTest — replaced Thread.sleep(250) and Thread.sleep(200) with await().until() checking JMX inflight exchange count
  • DefaultExecutorServiceManagerTest — replaced Thread.sleep(3000) with a started CountDownLatch + await() to detect when the pool task begins

Async/scheduler tests (2 files)

  • SchedulerMulticastParallelGreedyTest — replaced Thread.sleep(50) with MockEndpoint.setAssertPeriod(200) to verify no extra messages arrive
  • ThreadsRejectedExecutionWithDeadLetterTest — replaced Thread.sleep(100) with await().until() checking the rejected message arrived at dead letter

Aggregation tests (3 files)

  • AggregateGroupedExchangeBatchSizeTest — replaced Thread.sleep(1000) with await() for the second batch via completion timeout
  • AggregateDiscardOnTimeoutTest — replaced Thread.sleep(250) with MockEndpoint.setAssertPeriod(500)
  • AggregateClosedCorrelationKeyTest — replaced Thread.sleep(200) with await().until() checking all aggregated results arrived

Redelivery/shutdown tests (3 files)

  • NotAllowRedeliveryWhileStoppingTest, NotAllowRedeliveryWhileStoppingDeadLetterChannelTest — replaced Thread.sleep(500) with await() checking inflight repository
  • RedeliveryDeadLetterErrorHandlerNoRedeliveryOnShutdownTest — replaced Thread.sleep(500) with await().until(() -> counter.get() >= 20)

Resequencer test (1 file)

  • ResequenceStreamRejectOldExchangesTest — replaced Thread.sleep(100) with await().until() checking first message delivered before sending the rest

Out of scope

The following Thread.sleep() uses were intentionally left unchanged:

  • Sleeps inside Processor.process() or Runnable.run() simulating slow I/O
  • Timing-specific tests (completion intervals, rate calculations, timeout ordering)
  • Pacing sleeps between message sends where the delay is the test's intent

Claude Code on behalf of Guillaume Nodet

… camel-management tests

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gnodet gnodet requested review from davsclaus, orpiske and oscerd May 13, 2026 08:15
Comment on lines -66 to -71
// even though we wait for the queues to empty, there is a race condition where the consumer
// may still process messages while it's being suspended due to asynchronous message handling.
// as a result, we need to wait a bit longer to ensure that the seda consumer is suspended before
// sending the next message.
Thread.sleep(1000L);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no await replacement then this thread;sleep. This could only be worse than it currently is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the pollDelay(1, TimeUnit.SECONDS) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ef24703 — now split into two await() calls: first checks queues are empty, then uses pollDelay(1s) to give the consumer thread time to idle after the queues empty. The second await also asserts the consumer status is Suspended.

Claude Code on behalf of Guillaume Nodet

// may still process messages while it's being suspended due to asynchronous message handling.
// as a result, we need to wait a bit longer to ensure that the seda consumer is suspended before
// sending the next message.
Thread.sleep(1000L);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there isn't a wait condition anymore ensuring that the message is suspended

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the pollDelay(1, TimeUnit.SECONDS) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ef24703 — now split into two await() calls: first checks context.isSuspended(), then uses pollDelay(1s) to give the seda consumer thread time to complete its poll cycle before sending the test message.

Claude Code on behalf of Guillaume Nodet

// may still process messages while it's being suspended due to asynchronous message handling.
// as a result, we need to wait a bit longer to ensure that the seda consumer is suspended before
// sending the next message.
Thread.sleep(1000L);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no await replacement then this thread;sleep. This could only be worse than it currently is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the pollDelay(1, TimeUnit.SECONDS) ?

Copy link
Copy Markdown
Contributor

@apupier apupier May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The poll Delay is before checking for for the suspension and the big comment is explaining that we need to wait after this point due to async behavior

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the pollDelay was before the condition check, but the original Thread.sleep was after the queue-empty await. Fixed in ef24703 — now split into two steps:

  1. await().until(queue.isEmpty()) — checks the condition first
  2. await().pollDelay(1s).untilAsserted(...) — then gives the consumer thread time to idle

This preserves the original semantics: wait for the queue to empty, then wait for the consumer to stop its poll loop.

Claude Code on behalf of Guillaume Nodet

@github-actions
Copy link
Copy Markdown
Contributor

ghost commented May 13, 2026

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@gnodet gnodet requested a review from apupier May 13, 2026 08:38
Split single pollDelay-based await into two steps:
1. First await checks the queue-empty condition
2. Second await with pollDelay gives the consumer thread
   time to idle after the queue empties

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gnodet
Copy link
Copy Markdown
Contributor Author

gnodet commented May 13, 2026

Thanks for the review! You're right — the pollDelay was placed before the condition check, but the original Thread.sleep was after the queue-empty check. Fixed in ef24703: now using two-step await:

  1. First await().until(queue.isEmpty()) — checks the condition
  2. Second await().pollDelay(1s).untilAsserted(...) — gives the consumer thread time to idle after the queue empties

Claude Code on behalf of Guillaume Nodet

Use pollDelay(500ms) and atMost(2s) to match the original Thread.sleep(500)
timing and stay within the StopWatch 5-second budget.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@gnodet gnodet marked this pull request as draft May 13, 2026 10:45
… diverges

Move the inflight count, exchange ID, and duration assertions into
untilAsserted so Awaitility polls until the oldest exchange actually
changes, avoiding a race where durations are captured at the same value.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

ghost commented May 13, 2026

🧪 CI tested the following changed modules:

  • core/camel-core
  • core/camel-management

ℹ️ Dependent modules were not tested because the total number of affected modules exceeded the threshold (50). Use the test-dependents label to force testing all dependents.

⚠️ Some tests are disabled on GitHub Actions (@DisabledIfSystemProperty(named = "ci.env.name")) and require manual verification:

  • core/camel-core: 2 test(s) disabled on GitHub Actions
Build reactor — dependencies compiled but only changed modules were tested (2 modules)
  • Camel :: Core
  • Camel :: Management

⚙️ View full build and test results

@gnodet
Copy link
Copy Markdown
Contributor Author

gnodet commented May 13, 2026

Commits cherry-picked into #23178 which consolidates all flaky test fixes.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants