github-actions[bot] commented on code in PR #62117:
URL: https://github.com/apache/doris/pull/62117#discussion_r3036288816
##########
fe/fe-foundation/src/main/java/org/apache/doris/foundation/util/ConcurrentLong2ObjectHashMap.java:
##########
@@ -53,13 +53,26 @@
* interfaces (computeIfAbsent, computeIfPresent, compute, merge, putIfAbsent,
replace, remove)
* are overridden to ensure atomicity within a segment.
*
+ * <p><b>Callback restriction:</b> The mapping/remapping functions passed to
{@code computeIfAbsent},
+ * {@code computeIfPresent}, {@code compute}, and {@code merge} <em>must
not</em> access this map
+ * in any way while executing, including reads such as {@code get} or {@code
containsKey}, as well
+ * as writes. Reentrant write access from a callback is rejected at runtime
with
Review Comment:
Same issue here: the new `ThreadLocal` guard only protects write APIs.
Read-only access from inside `compute*` / `merge` callbacks is still allowed,
even though the class Javadoc now says callbacks must not access the map in any
way. Two threads doing cross-segment callback reads can still deadlock while
each holds a segment write lock.
##########
fe/fe-foundation/src/main/java/org/apache/doris/foundation/util/ConcurrentLong2LongHashMap.java:
##########
@@ -52,12 +52,22 @@
* <p><b>Important:</b> All compound operations from both {@link Long2LongMap}
and {@link Map}
* interfaces (computeIfAbsent, computeIfPresent, compute, merge, mergeLong,
putIfAbsent,
* replace, remove) are overridden to ensure atomicity within a segment.
+ *
+ * <p><b>Callback restriction:</b> The mapping/remapping functions passed to
{@code computeIfAbsent},
+ * {@code computeIfPresent}, {@code compute}, {@code merge}, and {@code
mergeLong} <em>must not</em>
+ * access this map in any way while executing, including reads such as {@code
get} or
+ * {@code containsKey}, as well as writes. Reentrant write access from a
callback is rejected at
Review Comment:
This still leaves the deadlock you describe in the new Javadoc.
`checkNotInCallback()` is only wired into mutating methods, so a callback can
still call `get()` / `containsKey()` / `size()` / snapshot iterators while the
current thread already holds one segment's write lock. With two threads running
`compute` on different segments and each callback reading the other key, you
get an ABBA `write(A) -> read(B)` / `write(B) -> read(A)` deadlock. Since the
contract now says callbacks must not access the map at all, the runtime guard
and tests need to cover read paths too, not just writes.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
