gnodet-bot commented on code in PR #26819:
URL: https://github.com/apache/camel/pull/26819#discussion_r4091050857
##########
core/camel-core-engine/src/main/java/org/apache/camel/impl/DefaultModel.java:
##########
@@ -66,9 +66,12 @@
import org.apache.camel.util.AntPathMatcher;
import org.apache.camel.util.ObjectHelper;
import org.apache.camel.util.StringHelper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class DefaultModel implements Model {
+ private static final Logger log =
LoggerFactory.getLogger(DefaultModel.class);
Review Comment:
🔴 **Dead code (source-check will fail):** `log` is declared but never used
in this PR. The two `import` lines at 69–70 are also dead. Remove all three, or
add actual logging (e.g. a `log.debug("addCustomBean: {}", bean.getName())`
inside the lock block — useful for diagnosing concurrent-deployment issues in
prod).
```suggestion
```
##########
core/camel-core-engine/src/main/java/org/apache/camel/impl/DefaultModel.java:
##########
@@ -901,9 +904,14 @@ public void setModelReifierFactory(ModelReifierFactory
modelReifierFactory) {
@Override
public void addCustomBean(BeanFactoryDefinition<?> bean) {
- // remove exiting bean with same name to update
- beans.removeIf(b -> bean.getName().equals(b.getName()));
- beans.add(bean);
+ lock.lock();
+ try {
+ // remove existing bean with same name to update; guard against
null entries left by prior corruption
+ beans.removeIf(b -> b != null &&
bean.getName().equals(b.getName()));
Review Comment:
⚠️ **`b != null` guard is symptom-masking.** An `ArrayList` managed under a
`ReentrantLock` cannot contain null entries — `beans.add(bean)` only adds the
non-null parameter, and `removeIf` with a matching predicate removes by value,
not by setting to null. If nulls appear, it means something outside the lock is
mutating the list — and the correct fix is to seal that access, not to skip
nulls here. If you switch to `CopyOnWriteArrayList` as suggested above, this
guard becomes both unnecessary and misleading — remove it.
##########
core/camel-core-engine/src/main/java/org/apache/camel/impl/DefaultModel.java:
##########
@@ -901,9 +904,14 @@ public void setModelReifierFactory(ModelReifierFactory
modelReifierFactory) {
@Override
public void addCustomBean(BeanFactoryDefinition<?> bean) {
- // remove exiting bean with same name to update
- beans.removeIf(b -> bean.getName().equals(b.getName()));
- beans.add(bean);
+ lock.lock();
+ try {
+ // remove existing bean with same name to update; guard against
null entries left by prior corruption
+ beans.removeIf(b -> b != null &&
bean.getName().equals(b.getName()));
+ beans.add(bean);
+ }finally {
Review Comment:
🔴 **Formatter gate will fail:** missing space between `}` and `finally`.
Camel's spotless/checkstyle enforces `} finally {`.
```suggestion
} finally {
```
##########
core/camel-core-engine/src/main/java/org/apache/camel/impl/DefaultModel.java:
##########
@@ -901,9 +904,14 @@ public void setModelReifierFactory(ModelReifierFactory
modelReifierFactory) {
@Override
public void addCustomBean(BeanFactoryDefinition<?> bean) {
- // remove exiting bean with same name to update
- beans.removeIf(b -> bean.getName().equals(b.getName()));
- beans.add(bean);
+ lock.lock();
+ try {
+ // remove existing bean with same name to update; guard against
null entries left by prior corruption
Review Comment:
⚠️ **Reader-side race not fixed.** The lock only covers the write path
(`addCustomBean`). `getCustomBeans()` returns the live `ArrayList` directly,
and callers in `BeanModelDevConsole` and `DefaultDumpRoutesStrategy` iterate it
with an enhanced-for loop:
```java
for (BeanFactoryDefinition bean : model.getCustomBeans()) { ... }
```
A concurrent `addCustomBean` mid-iteration will throw
`ConcurrentModificationException` regardless of this lock — the lock is not
held during the reader's iteration.
Simplest fix: switch `beans` to `CopyOnWriteArrayList`, which handles
concurrent writer/reader pairs atomically and lets you remove both the explicit
lock and the `b != null` guard:
```suggestion
beans.removeIf(b -> bean.getName().equals(b.getName()));
beans.add(bean);
```
##########
core/camel-support/src/main/java/org/apache/camel/support/SimpleRegistry.java:
##########
@@ -35,7 +36,7 @@
*
* @see DefaultRegistry
*/
-public class SimpleRegistry extends LinkedHashMap<String, Map<Class<?>,
Object>> implements Registry, Closeable {
+public class SimpleRegistry extends ConcurrentHashMap<String, Map<Class<?>,
Object>> implements Registry, Closeable {
Review Comment:
ℹ️ **Ordering contract change (worth documenting).** Switching from
`LinkedHashMap` to `ConcurrentHashMap` silently drops insertion order.
`findByType` and `findByTypeWithName` now return beans in an unspecified (hash)
order instead of registration order. This is unlikely to matter in practice
since lookups are by name/type, but any code relying on deterministic iteration
order for same-type beans will silently break. Consider adding a comment on the
class (or in the commit message) acknowledging this behavioural delta.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
