melloware commented on PR #1022:
URL: https://github.com/apache/myfaces/pull/1022#issuecomment-4758658078
here was the note from AI on computeIfAbstent
Yes, this is a race condition.
The bug is exactly here:
```java
for (int i = 0, size = eventInfos.size(); i < size; i++)
{
EventInfo eventInfo = eventInfos.get(i);
...
}
```
`eventInfos` is actually a `CopyOnWriteArrayList<EventInfo>`:
```java
eventInfos = new CopyOnWriteArrayList<>();
```
The problem is that `CopyOnWriteArrayList.size()` and `get()` are **not
atomic**. Consider:
```
Thread A Thread B
-------- --------
size = eventInfos.size(); // 3
unsubscribeFromEvent()
removeIf(...)
backing array now size = 2
eventInfos.get(2); // ArrayIndexOutOfBoundsException
```
`CopyOnWriteArrayList` intentionally replaces its internal array on every
modification. The `size()` came from one snapshot while `get()` reads from a
newer snapshot.
This is exactly the exception you're seeing:
```
Index 2 out of bounds for length 2
```
---
## The correct fix
The JDK documentation recommends iterating using the snapshot iterator.
Instead of
```java
for (int i = 0, size = eventInfos.size(); i < size; i++)
{
EventInfo eventInfo = eventInfos.get(i);
...
}
```
use
```java
for (EventInfo eventInfo : eventInfos)
{
if (eventInfo.sourceClass != null &&
!eventInfo.sourceClass.isAssignableFrom(sourceBaseType))
{
continue;
}
if (eventInfo.listener.isListenerForSource(source))
{
if (event == null)
{
event = createEvent(systemEventClass, facesContext, source);
}
if (event.isAppropriateListener(eventInfo.listener))
{
event.processListener(eventInfo.listener);
}
}
}
```
or equivalently
```java
for (Iterator<EventInfo> it = eventInfos.iterator(); it.hasNext();)
{
EventInfo eventInfo = it.next();
...
}
```
The iterator captures the backing array once, so it is immune to concurrent
additions/removals.
---
## There are other occurrences in this class
I also noticed `processComponentAttachedListeners()` contains the same
pattern:
```java
for (int i = 0, size = listeners.size(); i < size; i++)
{
SystemEventListener listener = listeners.get(i);
...
}
```
If `holder.getListenersForEventClass()` ever returns a
`CopyOnWriteArrayList` (which it commonly does in JSF implementations), this
method has the identical race.
`processViewAttachedListeners()` is already written defensively by making a
copy because of MYFACES-2935, so the author was clearly aware of concurrent
modification there but overlooked the other two methods.
---
## I would also fix `subscribeToEvent()`
There is another concurrency issue:
```java
List<EventInfo> eventInfos = globalListeners.get(systemEventClass);
if (eventInfos == null)
{
eventInfos = new CopyOnWriteArrayList<>();
globalListeners.put(systemEventClass, eventInfos);
}
```
Two threads can both create different lists:
```
T1: get() -> null
T2: get() -> null
T1: put(listA)
T2: put(listB)
T1: listA.add(listener1)
T2: listB.add(listener2)
```
One listener is effectively lost because `listA` is no longer referenced by
the map.
This should instead use:
```java
List<EventInfo> eventInfos =
globalListeners.computeIfAbsent(systemEventClass,
k -> new CopyOnWriteArrayList<>());
```
which atomically creates or retrieves the shared list.
So I think there are **two real concurrency bugs** in
`ApplicationImplEventManager`:
1. **`processGlobalListeners()`** (the one causing your exception) iterates
a `CopyOnWriteArrayList` incorrectly using `size()`/`get()`.
2. **`subscribeToEvent()`** can lose listener registrations under concurrent
subscriptions because it uses `get()`/`put()` instead of `computeIfAbsent()`.
Both are worth fixing in a MyFaces pull request.
--
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]
