[
https://issues.apache.org/jira/browse/TOMEE-4699?focusedWorklogId=1040583&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1040583
]
ASF GitHub Bot logged work on TOMEE-4699:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 09/Sep/26 18:59
Start Date: 09/Sep/26 18:59
Worklog Time Spent: 10m
Work Description: rzo1 commented on code in PR #2939:
URL: https://github.com/apache/tomee/pull/2939#discussion_r3971992209
##########
container/openejb-core/src/main/java/org/apache/openejb/core/ThreadContext.java:
##########
@@ -237,9 +239,15 @@ public String toString() {
}
private String dataToString(final Map<Class, Object> data) {
- return data.entrySet().stream()
+ // copy data under monitor (synchronized map), format outside lock
+ return synchronizedCopy(data).entrySet().stream()
.map(entry -> entry.getKey() + "=" + (entry.getValue() == null
? "null" : entry.getValue().hashCode()))
.collect(Collectors.joining(", "));
+ }
Review Comment:
Keeping the copy is the safer option here (imho): that monitor is the same
one `get`/`set`/`remove` take, and `InterceptorStack` takes it around every
business method, so holding it across `entry.getValue().hashCode()` puts code
we don't control on the EJB invocation path. No current value in that map does
anything expensive in `hashCode()`, so this is about not depending on that, and
formatting outside the monitor only costs one small `HashMap` copy on a
debug-only path.
`toString()` reads `data.size()` outside the lock and then `dataToString`
locks again, so the printed count and the printed entries come from two
different reads. Folding the size into the same copy fixes that:
```java
private String dataToString(final Map<Class, Object> data) {
final Map<Class, Object> copy = synchronizedCopy(data);
return "(" + copy.size() + ")=" + copy.entrySet().stream()
.map(entry -> entry.getKey() + "=" + (entry.getValue() == null ?
"null" : entry.getValue().hashCode()))
.collect(Collectors.joining(", "));
}
```
with `toString()` using `", data=" + dataToString(data)`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1040583)
Time Spent: 50m (was: 40m)
> ThreadContext constructor is not thread-safe, causing loss of submitted tasks.
> ------------------------------------------------------------------------------
>
> Key: TOMEE-4699
> URL: https://issues.apache.org/jira/browse/TOMEE-4699
> Project: TomEE
> Issue Type: Bug
> Components: TomEE Core Server
> Affects Versions: 10.2.0
> Reporter: Jorren Hendriks
> Priority: Major
> Fix For: 11.0.0, 10.3.0
>
> Attachments: stacktrace.txt
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Some of our tomee applications have experienced issues losing tasks submitted
> to a managed executor.
> The root cause has been identified as a
> `{_}ConcurrentModificationException{_}` in the ThreadContext constructor
> (stacktrace attached).
> {code:java}
> public ThreadContext(final ThreadContext that) {
> this.beanContext = that.beanContext;
> this.primaryKey = that.primaryKey;
> this.data.putAll(that.data); // throws ConcurrentModificationException
> when that.data is modified on another thread
> this.oldClassLoader = that.oldClassLoader;
> }{code}
> `{_}HashMap.putAll{_}` will iterate the entrySet, which is not thread-safe on
> the `{_}Collections.synchronizedMap{_}` wrapped HashMap currently in use.
> A possible solution would be to use a thread-safe Map implementation.
> This constructor is called when entering the snapshot in the
> [{_}ApplicationThreadContextProvider{_}|[https://github.com/apache/tomee/blob/main/container/openejb-core/src/main/java/org/apache/openejb/threads/impl/ApplicationThreadContextProvider.java#L76|https://github.com/apache/tomee/blob/main/container/openejb-core/src/main/java/org/apache/openejb/threads/impl/ApplicationThreadContextProvider.java#L76].]]
> This issue also revealed possible other issues for us:
> 1. `{_}CUTask{_}` does not handle exceptions before starting the task. Any
> exceptions in `{_}contextService.enter(snapshot){_}` cancel invocation, but
> do not abort the task.
> 2. Exceptions in a `{_}CUTask{_}`/`{_}CURunnable{_}` are propagated to the
> `{_}ThreadPoolExecutor{_}`. The current implementation does not expose these
> exceptions which makes this a silent failure.
> If you'd prefer that I can file these as separate issues, but they are
> related to this problem.
> h2. Reproduction
> This is a race condition which very rarely occurs. We were able to reproduce
> it (under high load) within an hour on one of our applications with a
> debugger attached. So far we were not able to create a minimal reproduction
> outside our application. I will update the issue when we do.
> The pattern we use in the application is as follows, the comments indicate my
> interpretation of what happens:
> {code:java}
> // MyTaskExecutor.java
> @Stateless
> public class MyTaskExecutor {
> @Resource
> private ManagedExecutorService managedExecutorService; // default TomEE
> managed executor service
> @Override
> public void execute(@NotNull Runnable runnable) {
> // 1. runnable is wrapped in CUTask. ThreadContext is copied on the
> executor thread.
> managedExecutorService.submit(runnable);
> } // 2. method returns immediately, causing an update to the current
> ThreadContext.
> } {code}
> When the exception occurs, 1 & 2 both execute at the same time, accessing the
> current ThreadContext concurrently.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
