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)`.
--
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]
