dataroaring commented on code in PR #62117:
URL: https://github.com/apache/doris/pull/62117#discussion_r3064357067
##########
fe/fe-foundation/src/main/java/org/apache/doris/foundation/util/ConcurrentLong2LongHashMap.java:
##########
@@ -52,12 +52,20 @@
* <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>
+ * attempt to update any other mappings of this map. This restriction is
enforced at runtime:
+ * reentrant access from a callback throws {@link IllegalStateException}.
Violation may also cause
+ * deadlock if callbacks attempt cross-segment updates from multiple threads.
Review Comment:
Addressed in commit 64db7d27. Same fix in the Long2Long map:
`checkNotInCallback()` added to all read methods (`get`, `getOrDefault`,
`containsKey`, `containsValue`, `size`, `isEmpty`). Test
`testReentrantReadFromCallbackThrowsIllegalStateException` covers this.
— ThinkOps 🤖
##########
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:
Addressed in commit 64db7d27. `checkNotInCallback()` now guards all read
methods (`get`, `getOrDefault`, `containsKey`, `containsValue`, `size`,
`isEmpty`) in addition to write methods. Any map access from within a
compute/merge callback — read or write — now throws `IllegalStateException`,
fully closing the ABBA cross-segment deadlock window. Test coverage added via
`testReentrantReadFromCallbackThrowsIllegalStateException`.
— ThinkOps 🤖
##########
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:
Addressed in commit 64db7d27. Same fix applied here: the `ThreadLocal` guard
now covers both read and write APIs. All six read methods (`get`,
`getOrDefault`, `containsKey`, `containsValue`, `size`, `isEmpty`) call
`checkNotInCallback()` at entry. The documented contract (callbacks must not
access the map in any way) is now enforced at runtime for all code paths.
— ThinkOps 🤖
--
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]
