-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Another set of code related warnings fixes #14222
Conversation
gnodet
commented
May 22, 2024
- Fix visibility issues
- Fix busy-wait loop issues
🌟 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:
|
There's another busy-wait loop issue in |
...el-core-processor/src/main/java/org/apache/camel/processor/aggregate/AggregateProcessor.java
Outdated
Show resolved
Hide resolved
...l-core-processor/src/main/java/org/apache/camel/processor/resequencer/ResequencerEngine.java
Show resolved
Hide resolved
Upon further investigation, the potential problem comes from this commit: |
The key problem I was avoiding on 07b9c68ea2a9a70cf3a3f42c1045fb9fd0acbe6a, was the The STARTING state check had been there before. Looking at the history of this change, it seems that the STARTING state check is related to CAMEL-14048 and a problem with Netty. |
@gnodet it would probably be good to run a full test on the code before merging. I've triggered a build on my own CI to see how it goes. |
Thx for the pointers. I've had another look, I think the fix 0f9f879 is wrong and the problem was actually fixed correctly by 4c8c071. This commit makes sure that the double-check locking only see a fully started service. I'll add a commit to remove that part of the code. |
Agreed. The PR template mentions |
Pushed a commit. However I think the |
I think these labels are outdated. IIRC, we never managed to run full builds on GH because it takes too long and causes the action runner to abort after some time. For now, the only way to run a full test is by running the tests manually (or, semi automatically, as I do on my environment). |
First run (before the last commit pushed) had only 1 failure on I have a new build running with the latest commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested on my own CI and there were only 2 failures which are known flakies:
- org.apache.camel.component.file.FileSortByExpressionTest.testSortFiles
- org.apache.camel.component.rocketmq.RocketMQRouteIT.testSimpleRoute
The service is always in a started state. The problem was caused by the field being set (while holding the lock) to the newly created service before it was actually started, so that another thread could see the non started service.