[
https://issues.apache.org/jira/browse/CAMEL-19549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090936#comment-18090936
]
Torsten Mielke edited comment on CAMEL-19549 at 6/25/26 9:24 AM:
-----------------------------------------------------------------
[~orpiske] I had a look at this in the last day.
There are 56 Test classes in {{core/camel-core}} that use {{Thread.sleep()}} in
some form or another. So it is quite an extensive review that would be needed
here.
I tried to involve an AI agent but it ...
- repeatedly suggested far more complex alternatives to {{Thread.sleep()}} in
cases where the intend was truly to let some time pass and not to coordinate
concurrent actions,
- did not detect all possible improvements
So I started to review test classes more manually and found that most
occurrences of {{Thread.sleep()}} that I saw were indeed to ensure some time
passes before the next event and not to coordinate actions.
Files I reviewed where the use of Thread.sleep() seems legitimate
- {{src/test/java/org/apache/camel/impl/StopTimeoutRouteTest.java}}
sleep is used to test stopping the route while an exchange is still in
progress
-
{{src/test/java/org/apache/camel/impl/ShutdownStrategyNotSuppressLoggingOnTimeoutManualTest.java}}
manual test where sleep is used to test shut down strategy while exchange is
being processed
-
{{src/test/java/org/apache/camel/impl/ShutdownStrategySuppressLoggingOnTimeoutManualTest.java}}
manual test where sleep is used to test shut down strategy while exchange is
being processed
- {{src/test/java/org/apache/camel/impl/engine/PeriodTaskSchedulerTest.java}}
sleep used to allow some time to pass between events
- {{src/test/java/org/apache/camel/util/CaseInsensitiveMapTest.java}}
manual test with explicit sleep call to take heap dumps
- {{src/test/java/org/apache/camel/processor/PipelineConcurrentTest.java}}
sleep used to generate a more random load
-
{{src/test/java/org/apache/camel/processor/enricher/PollEnrichFile.DefaultAggregationStrategyTest.java}}
sleep used to allow some time to pass between events
- {{src/test/java/org/apache/camel/processor/enricher/PollTest.java}}
sleep used to allow some time to pass between events
- {{src/test/java/org/apache/camel/processor/enricher/PollEnricherTest.java}}
sleep used to allow some time to pass between events
-
{{src/test/java/org/apache/camel/processor/SplitterParallelBigFileManualTest.java}}
manual test with explicit sleep call to take heap dumps
-
{{src/test/java/org/apache/camel/processor/resequencer/ResequencerRunner.java}}
legitimate use of configurable Thread.sleep(). This class is used by another
test.
-
{{src/test/java/org/apache/camel/processor/aggregator/AggregateTimeoutManualTest.java}}
disabled and manual test, though I don't think the sleep() is needed there.
-
{{src/test/java/org/apache/camel/processor/aggregator/AggregateGroupedExchangeMultipleCorrelationTest.java}}
uses Thread.sleep() to enforce a timeout, seems legitimate
- {{src/test/java/org/apache/camel/processor/async/MyAsyncProducer.java}}
Simulates a task which takes some millis to reply, legitimate
- {{src/test/java/org/apache/camel/processor/VirtualThreadsLoadTest.java}}
also simulates IO delay using sleep(), legitimate
-
{{src/test/java/org/apache/camel/processor/VirtualThreadsWithThreadsDSLLoadTest.java}}
also simulates IO delay using sleep(), legitimate
- {{src/test/java/org/apache/camel/processor/StreamResequencerTest.java}}
delays msg send randomly and on purpose, legitimate
- {{src/test/java/org/apache/camel/processor/WireTapShutdownBeanTest.java}}
enforces delay when sending msgs, already uses awaitility to shut down test
when done
-
{{src/test/java/org/apache/camel/processor/MultiCastParallelAndStreamCachingTest.java}}
uses sleep() to simulate more realistic load
- {{src/test/java/org/apache/camel/processor/SamplingThrottlerTest.java}}
legitimate to enforce new msgs being send after the sample period has elapsed
-
{{src/test/java/org/apache/camel/component/file/FileBatchConsumerMemoryLeakManualTest.java}}
manual test, sleep is used to keep the JVM running after test in order to
manually collect JVM info
I then gave up as these uses of {{Thread.sleep()}} all seem legitimate to me.
Based on the analysis so far I wonder if it's worth to manually review the
other 40+ test classes?
I would not trust AI agents too much here, so do not always understand the
concurrency of the tests correctly.
I had suggestions from the agent that when challenged was completely withdrawn
again by the agent.
Nevertheless, I do have some improvements to suggest in a few files but it
would not be a full review of all the test classes.
Does it make sense to open a PR for the changes I have so far, even though it
won't resolve this ticket entirely?
was (Author: tmielke):
[~orpiske] I had a look at this in the last day.
There are 56 Test classes in {{core/camel-core}} that use {{Thread.sleep()}} in
some form or another. So it is quite an extensive review that would be needed
here.
I tried to involve an AI agent but it ...
- repeatedly suggested far more complex alternatives to {{Thread.sleep()}} in
cases where the intend was truly to let some time pass and not to coordinate
concurrent actions,
- did not detect all possible improvements
So I started to review test classes more manually and found that most
occurrences of {{Thread.sleep()}} that I saw were indeed to ensure some time
passes before the next event and not to coordinate actions.
Files I reviewed where the use of Thread.sleep() seems legitimate
- {{src/test/java/org/apache/camel/impl/StopTimeoutRouteTest.java}}
sleep is used to test stopping the route while an exchange is still in
progress
-
{{src/test/java/org/apache/camel/impl/ShutdownStrategyNotSuppressLoggingOnTimeoutManualTest.java}}
manual test where sleep is used to test shut down strategy while exchange is
being processed
-
{{src/test/java/org/apache/camel/impl/ShutdownStrategySuppressLoggingOnTimeoutManualTest.java}}
manual test where sleep is used to test shut down strategy while exchange is
being processed
- {{src/test/java/org/apache/camel/impl/engine/PeriodTaskSchedulerTest.java}}
sleep used to allow some time to pass between events
- {{src/test/java/org/apache/camel/util/CaseInsensitiveMapTest.java}}
manual test with explicit sleep call to take heap dumps
- {{src/test/java/org/apache/camel/processor/PipelineConcurrentTest.java}}
sleep used to generate a more random load
-
{{src/test/java/org/apache/camel/processor/enricher/PollEnrichFile.DefaultAggregationStrategyTest.java}}
sleep used to allow some time to pass between events
- {{src/test/java/org/apache/camel/processor/enricher/PollTest.java}}
sleep used to allow some time to pass between events
- {{src/test/java/org/apache/camel/processor/enricher/PollEnricherTest.java}}
sleep used to allow some time to pass between events
-
{{src/test/java/org/apache/camel/processor/SplitterParallelBigFileManualTest.java}}
manual test with explicit sleep call to take heap dumps
I then gave up as these uses of {{Thread.sleep()}} all seem legitimate to me.
Based on the analysis so far I wonder if it's worth to manually review the
other 40+ test classes?
I would not trust AI agents too much here, so do not always understand the
concurrency of the tests correctly.
I had suggestions from the agent that when challenged was completely withdrawn
again by the agent.
Nevertheless, I do have some improvements to suggest in a few files but it
would not be a full review of all the test classes.
Does it make sense to open a PR for the changes I have so far, even though it
won't resolve this ticket entirely?
> camel-core: replace Thread.sleep in tests
> -----------------------------------------
>
> Key: CAMEL-19549
> URL: https://issues.apache.org/jira/browse/CAMEL-19549
> Project: Camel
> Issue Type: Task
> Components: camel-core, tests
> Affects Versions: 4.0.0
> Reporter: Otavio Rodolfo Piske
> Assignee: Torsten Mielke
> Priority: Minor
> Labels: easy, help-wanted
>
> We have many tests which use Thread.sleep for synchronization. This is bug
> prone and can introduce flakiness when running on environments with different
> capacities.
> Ideally we should replace these with:
> * [Awaitility|http://www.awaitility.org/]
> * Java's native syncronization mechanism (Latches, Phasers, Locks, etc)
> * Nothing (i.e.; in some cases the sleep can simply be removed)
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
