feat: make LIST_ARRAY_GETTER optional #1230
Conversation
Signed-off-by: Dennis Zhuang <[email protected]>
WalkthroughRefactors SegmentList to lazily initialize a reflection-based ArrayList elementData updater with null-guarded usage in coll2Array, falling back to toArray() if unavailable. Removes ThrowUtil’s causeUpdater and the cutCause method, leaving throwException and getRootCause intact. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant SL as SegmentList
participant AL as ArrayList
participant RFU as RefFieldUpdater (elementData)
Note over SL: coll2Array(Collection<?> coll)
C->>SL: coll2Array(coll)
alt coll is ArrayList AND RFU initialized AND unsafe allowed
SL->>RFU: get(ArrayList.elementData)
RFU-->>SL: Object[] elementData
SL-->>C: return elementData copy/slice
else
SL->>AL: coll.toArray()
AL-->>SL: Object[] arr
SL-->>C: return arr
end
%% Styling
rect rgba(230,245,255,0.6)
Note over RFU,SL: New guarded fast-path (reflection-based)
end
sequenceDiagram
autonumber
participant U as Upstream code
participant TU as ThrowUtil
Note over TU: Before: cutCause(...) existed
U-xTU: cutCause(Throwable)
Note right of TU: Method removed
U->>TU: getRootCause(Throwable)
TU-->>U: Throwable root
U->>TU: throwException(Throwable)
TU-->>U: (throws)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Modifies the LIST_ARRAY_GETTER field updater initialization to be optional, preventing compilation errors in newer JDK versions by gracefully handling cases where reflection access to ArrayList's internal field may fail.
- Made
LIST_ARRAY_GETTERfield nullable and initialized through a try-catch block - Added null check before using the field updater in
coll2Arraymethod - Removed unused
cutCausemethod and associated field updater fromThrowUtilclass
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ThrowUtil.java | Removed unused cutCause method and causeUpdater field |
| SegmentList.java | Made LIST_ARRAY_GETTER initialization optional with error handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| try { | ||
| LIST_ARRAY_GETTER = Updaters.newReferenceFieldUpdater(ArrayList.class, "elementData"); | ||
| } catch (Throwable t) { | ||
| //ignore |
There was a problem hiding this comment.
The catch block comment should explain why the exception is ignored and what fallback behavior occurs when LIST_ARRAY_GETTER is null.
| //ignore | |
| // Failed to create LIST_ARRAY_GETTER (likely due to reflection restrictions). | |
| // Fallback: LIST_ARRAY_GETTER remains null, so coll2Array will use coll.toArray(), | |
| // which is less efficient but still correct. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
jraft-core/src/main/java/com/alipay/sofa/jraft/util/SegmentList.java (2)
430-438: Make the updater immutable and gate initialization once in the static blockMark the field final and decide availability up front (also avoids an extra Unsafe check on every call). Keeps mutability to zero and semantics unchanged.
Apply:
- private static ReferenceFieldUpdater<ArrayList<?>, Object[]> LIST_ARRAY_GETTER = null; + private static final ReferenceFieldUpdater<ArrayList<?>, Object[]> LIST_ARRAY_GETTER; @@ - static { - try { - LIST_ARRAY_GETTER = Updaters.newReferenceFieldUpdater(ArrayList.class, "elementData"); - } catch (Throwable t) { - //ignore - } - } + static { + ReferenceFieldUpdater<ArrayList<?>, Object[]> getter = null; + if (UnsafeUtil.hasUnsafe()) { + try { + getter = Updaters.newReferenceFieldUpdater(ArrayList.class, "elementData"); + } catch (Throwable t) { + // ignore: fall back to toArray() + } + } + LIST_ARRAY_GETTER = getter; + }
465-469: Avoid unchecked cast and short‑circuit the cheapest condition firstUse ArrayList<?> to remove the unchecked cast warning and check instanceof first. No behavior change.
- if (LIST_ARRAY_GETTER != null && coll instanceof ArrayList && UnsafeUtil.hasUnsafe()) { - src = LIST_ARRAY_GETTER.get((ArrayList<T>) coll); + if (coll instanceof ArrayList && LIST_ARRAY_GETTER != null && UnsafeUtil.hasUnsafe()) { + src = LIST_ARRAY_GETTER.get((ArrayList<?>) coll); } else { src = coll.toArray(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jraft-core/src/main/java/com/alipay/sofa/jraft/util/SegmentList.java(2 hunks)jraft-core/src/main/java/com/alipay/sofa/jraft/util/internal/ThrowUtil.java(0 hunks)
💤 Files with no reviewable changes (1)
- jraft-core/src/main/java/com/alipay/sofa/jraft/util/internal/ThrowUtil.java
🧰 Additional context used
🧬 Code graph analysis (1)
jraft-core/src/main/java/com/alipay/sofa/jraft/util/SegmentList.java (1)
jraft-core/src/main/java/com/alipay/sofa/jraft/util/internal/Updaters.java (1)
Updaters(24-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sca
Motivation:
Explain the context, and why you're making that change.
To make others understand what is the problem you're trying to solve.
Modification:
Make the
LIST_ARRAY_GETTERoptional to avoid compilation error in latest jdk.Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.
Summary by CodeRabbit