[
https://issues.apache.org/jira/browse/GEODE-10519?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jinwoo Hwang updated GEODE-10519:
---------------------------------
Description:
h2. Summary
UnsafeThreadLocal currently uses reflection to access private internals of
{{java.lang.ThreadLocal}} and {{{}java.lang.ThreadLocalMap{}}}, requiring the
JVM flag {{{}--add-opens=java.base/java.lang=ALL-UNNAMED{}}}. This creates
maintenance burden, security concerns, and compatibility risks with future Java
versions.
h2. Description
h3. Current Implementation
The {{UnsafeThreadLocal}} class (located in
{{{}geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java{}}})
extends {{ThreadLocal<T>}} and provides a {{get(Thread)}} method that allows
reading ThreadLocal values from arbitrary threads - a capability needed for
deadlock detection.
The current implementation uses reflection to:
# Access the private {{getMap(Thread)}} method of ThreadLocal
# Access the private {{getEntry(ThreadLocal)}} method of ThreadLocalMap
# Access the private {{value}} field of ThreadLocalMap.Entry
*Code excerpt showing reflection usage:*
{code:java}
private static Object get(ThreadLocal threadLocal, Thread thread) {
try {
Object threadLocalMap = invokePrivate(threadLocal, "getMap",
new Class[] {Thread.class}, new Object[] {thread});
if (threadLocalMap != null) {
Object entry = invokePrivate(threadLocalMap, "getEntry",
new Class[] {ThreadLocal.class}, new Object[] {threadLocal});
if (entry != null) {
return getPrivate(entry, "value");
}
}
return null;
} catch (Exception e) {
throw new RuntimeException("Unable to extract thread local", e);
}
}
{code}
h3. Required JVM Flags
To enable this reflection access, Geode requires the following JVM flag
(defined in {{{}MemberJvmOptions.java{}}}):
{code:java}
private static final String JAVA_LANG_OPEN =
"--add-opens=java.base/java.lang=ALL-UNNAMED";
{code}
This flag must be passed to every Geode member JVM, adding operational
complexity.
h3. Usage Context
UnsafeThreadLocal is used in three key areas for deadlock detection:
# *DLockService* ({{{}geode-core/.../locks/DLockService.java{}}})
** Tracks what distributed lock a thread is blocked on
** Used by: {{private final UnsafeThreadLocal<Object> blockedOn}}
# *MessageDependencyMonitor*
({{{}geode-core/.../deadlock/MessageDependencyMonitor.java{}}})
** Tracks ReplyProcessor21 instances threads are waiting for
** Tracks MessageWithReply instances being processed
** Used by: {{private final UnsafeThreadLocal<ReplyProcessor21>
waitingProcessors}}
** Used by: {{private final UnsafeThreadLocal<MessageWithReply>
processingMessages}}
# *DLockDependencyMonitor*
({{{}geode-core/.../deadlock/DLockDependencyMonitor.java{}}})
** Queries blockedOn status from DLockService for dependency graph construction
h2. Problems with Current Approach
h3. 1. Java Module System Violations
*Severity: High*
The reflection-based approach directly violates the Java Platform Module System
(JPMS) encapsulation:
* Accesses internal implementation details of {{java.lang}} package
* Bypasses the module system's access controls
* Requires explicit {{--add-opens}} directive to override module boundaries
{*}Risk{*}: Future Java versions may further restrict or remove the ability to
open internal packages, even with command-line flags.
h3. 2. Maintenance and Fragility
*Severity: High*
The implementation is brittle and depends on:
* Internal class names remaining unchanged (ThreadLocalMap, Entry)
* Internal method signatures staying stable (getMap, getEntry)
* Internal field names not changing (value)
* Class hierarchy not being refactored
Any changes to ThreadLocal internals in future JDK releases could break Geode
without warning, as these are non-public APIs with no stability guarantees.
h3. 3. Security Concerns
*Severity: Medium*
Using {{{}--add-opens=java.base/java.lang=ALL-UNNAMED{}}}:
* Opens the entire {{java.lang}} package to all unnamed modules
* Grants broader access than necessary (only need ThreadLocal access)
* Weakens Java's security model and encapsulation guarantees
* May trigger security audit flags in enterprise environments
h3. 4. Operational Complexity
*Severity: Medium*
Current deployment requirements:
* JVM flags must be correctly configured on all Geode servers
* Different flags needed for Java 8 vs. Java 11+ deployments
* Documentation and deployment scripts must maintain flag lists
* Missing flags cause runtime failures that are difficult to diagnose
>From {{{}MemberJvmOptions.java{}}}:
{code:java}
static final List<String> JAVA_11_OPTIONS = Arrays.asList(
COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
SUN_NIO_CH_EXPORT,
COM_SUN_MANAGEMENT_INTERNAL_OPEN,
JAVA_LANG_OPEN, // ← Required only for UnsafeThreadLocal
JAVA_NIO_OPEN);
{code}
h3. 5. Performance Overhead
*Severity: Low*
Reflection calls have performance costs:
* Method lookup via reflection on each invocation
* Access permission checks
* Potential JIT optimization barriers
* Exception handling overhead
While not critical for deadlock detection (which is infrequent), it's still
unnecessary overhead.
h3. 6. Testing and CI/CD Complexity
*Severity: Low*
Test infrastructure must:
* Configure JVM flags correctly for all test executions
* Maintain separate flag lists for different Java versions
* Handle flag propagation in parallel test runs
* Document requirements for external contributors
h2. Business Impact
h3. Reduced Operational Risk
Eliminating the need for {{--add-opens}} flags:
* Simplifies deployment procedures
* Reduces configuration errors in production
* Removes a potential point of failure during Java upgrades
* Improves compatibility with container orchestration platforms
h3. Better Java Version Compatibility
A non-reflection implementation:
* Works seamlessly across all Java versions (8, 11, 17, 21+)
* Doesn't depend on JDK internal implementation details
* Reduces risk during major Java version upgrades
* Aligns with Java's "code to interfaces, not implementations" principle
h3. Enhanced Security Posture
Removing reflection-based access:
* Eliminates the need to weaken module system protections
* Reduces attack surface by not opening internal packages
* Simplifies security audits and compliance reviews
* Follows principle of least privilege
h3. Improved Developer Experience
A cleaner implementation:
* Easier to understand and maintain
* No special JVM configuration needed for development
* Simpler IDE setup and debugging
* Reduces onboarding friction for new contributors
h3. Long-Term Maintainability
Moving away from internal APIs:
* Protects against breaking changes in future JDK releases
* Reduces technical debt
* Makes the codebase more sustainable
* Demonstrates modern Java best practices
h2. Affected Components
h3. Files Modified in This Refactoring
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java}}
*
{{geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java}}
h3. Files Using UnsafeThreadLocal (Not Modified)
These files use {{UnsafeThreadLocal}} but require no changes as the public API
remains unchanged:
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java}}
- Uses {{UnsafeThreadLocal<Object> blockedOn}} for deadlock detection
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/MessageDependencyMonitor.java}}
- Uses two UnsafeThreadLocal instances for tracking message dependencies
*
{{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/DLockDependencyMonitor.java}}
- Queries UnsafeThreadLocal values for dependency graph construction
h3. Test Files (Executed, Not Modified)
*
{{geode-core/src/test/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocalJUnitTest.java}}
- All tests pass with new implementation
h2. Scope
* Refactoring UnsafeThreadLocal implementation to eliminate reflection
* Updating MemberJvmOptions to remove JAVA_LANG_OPEN flag
* Ensuring all existing tests pass with new implementation
* Maintaining exact same public API and behavior
* Supporting all currently supported Java versions (17, 21)
h2. Testing Requirements
h3. Functional Testing
* All existing UnsafeThreadLocal unit tests must pass
* Deadlock detection integration tests must pass
* DLock-related distributed tests must pass
* Message processing tests must pass
h3. Cross-Version Testing
* Verify functionality on Java 17, and 21
* Ensure no regression in deadlock detection accuracy
* Validate thread safety under concurrent access
h3. Performance Testing
* Verify no performance regression in deadlock detection
* Measure overhead reduction from removing reflection
* Test under high concurrency scenarios
h3. Negative Testing
* Verify proper cleanup when threads terminate
* Test behavior with many ThreadLocal instances
* Validate memory leak prevention
h2. Compatibility
h3. Backward Compatibility
* Public API of UnsafeThreadLocal must remain unchanged
* Behavior must be functionally equivalent to current implementation
* No changes to deadlock detection output or timing
h3. Forward Compatibility
* Implementation should work with future Java versions
* Should not depend on version-specific features
* Should gracefully handle JVM updates
h2. Priority Assessment
*Recommended Priority: Medium-High*
h3. Factors Supporting High Priority
* Eliminates security concerns with module system violations
* Improves long-term maintainability significantly
* Reduces operational complexity
* Removes technical debt that will only grow over time
h3. Factors Supporting Medium Priority
* Current implementation works (with JVM flags)
* Not causing immediate production issues
* Other reflection usage in Geode also needs addressing
* Requires careful testing of deadlock detection
h2. Acceptance Criteria
# UnsafeThreadLocal no longer uses reflection to access ThreadLocal internals
# No {{--add-opens=java.base/java.lang=ALL-UNNAMED}} flag required
# All existing tests pass without modification
# Deadlock detection functionality unchanged
# Works on Java 11, 17, and 21 without special configuration
# Performance is equal or better than current implementation
# No memory leaks when threads are terminated
# Code is well-documented explaining the new approach
h2. Related Issues
* Related to any other tickets addressing reflection usage in Geode
* May be part of larger "Java Module System Compliance" initiative
was:
h2. Summary
Restore Gradle execution optimizations for the aggregated test reporting
workflow by explicitly declaring all upstream Test tasks as both inputs and
dependencies of the {{:combineReports}} TestReport task, eliminating validation
warnings and improving build performance.
h2. Description
h3. Problem Statement
The Gradle build system is emitting a validation warning that disables
execution optimizations for the {{:combineReports}} task, which aggregates test
results from all subprojects into a single HTML report. This warning indicates
implicit file coupling without proper task dependency declaration, causing
Gradle to defensively disable performance optimizations.
*Gradle Warning Message:*
{noformat}
Task ':combineReports' uses the output of task ':geode-old-versions:test'
without declaring an explicit or implicit dependency.
Execution optimizations have been disabled.
{noformat}
h3. Current Behavior
The {{combineReports}} task is configured to read binary test result
directories generated by multiple subproject Test tasks, but lacks formal task
dependency edges and input relationships:
{code:groovy}
// Current problematic configuration (simplified)
task combineReports(type: TestReport) {
// Reads test results from all subprojects
// BUT: No explicit dependency or input declaration
destinationDir = file("$buildDir/reports/combined")
}
{code}
*What Happens:*
# {{combineReports}} implicitly consumes test result outputs from subprojects
# Gradle 7+ detects this implicit file coupling
# Gradle cannot guarantee correct execution order
# Gradle defensively disables execution optimizations
# Warning appears in build logs (local and CI)
h3. Impact Assessment
h4. Build Performance
*Execution Optimization Disabled:*
* Reduced scheduling efficiency in multi-module builds
* Potential loss of incremental build behavior
* Suboptimal task parallelization
* Longer CI/CD pipeline execution times
*Quantified Impact (Estimated):*
* Multi-module builds: 5-10% slower scheduling
* Incremental builds: Up-to-date checks may not function optimally
* Parallel execution: Task graph optimization disabled for affected tasks
h4. Build Reliability
*Nondeterministic Ordering Risk:*
* Without explicit dependencies, task execution order is undefined
* {{:combineReports}} might execute before test tasks complete
* Race condition: Report may be generated with incomplete test results
* Build output may vary between runs (flaky builds)
*Symptoms:*
* Intermittent missing test results in combined report
* "File not found" errors if test results haven't been generated yet
* Different behavior in local vs. CI environments
h4. Developer Experience
*Noise and Confusion:*
* Warning appears in every build output
* Developers unsure if warning indicates a real problem
* CI logs cluttered with validation warnings
* Reduced signal-to-noise ratio in build diagnostics
h3. Root Cause Analysis
h4. Gradle Execution Model
Gradle builds a task dependency graph based on:
# *Explicit dependencies:* {{{}dependsOn{}}}, {{{}mustRunAfter{}}},
{{shouldRunAfter}}
# *Input/Output relationships:* Tasks declare what they consume and produce
# *Implicit dependencies:* Gradle infers dependencies from input/output wiring
*The Problem:*
The {{combineReports}} task:
* Reads files produced by Test tasks (implicit consumption)
* Does not declare these Test tasks as dependencies (no explicit edge)
* Does not declare test result directories as inputs (no input relationship)
Result: Gradle detects implicit file coupling but cannot safely optimize
scheduling.
h4. Why Gradle 7+ Is Stricter
*Historical Context:*
|Gradle Version|Behavior|Risk|
|Gradle 6.x|Tolerates implicit dependencies|Silent failures, nondeterministic
builds|
|Gradle 7.0+|Warns and disables optimizations|Conservative, safe, but slower|
|Gradle 8.0+|May fail builds with strict validation|Forces correct dependency
modeling|
*Philosophy Shift:*
* Gradle 7+ prioritizes build correctness over backward compatibility
* Implicit file coupling is treated as a configuration error
* Explicit is better than implicit (Python PEP 20 principle)
h4. Specific Issue: geode-old-versions Module
The {{:geode-old-versions:test}} task is particularly problematic:
* Located in a separate subproject ({{{}geode-old-versions{}}})
* Generates test results consumed by {{:combineReports}} in root project
* Cross-project dependency not declared
* Most frequently triggers the validation warning
h3. Scope of Issue
h4. Affected Files
{noformat}
build.gradle (root project)
└── combineReports task configuration
├── Missing: reportOn declaration
├── Missing: dependsOn declaration
└── Redundant: duplicate whenReady block
{noformat}
h4. Affected Workflows
# *Local development builds:* {{./gradlew build}}
# *CI/CD pipelines:* All test jobs generating combined reports
# *Developer testing:* {{./gradlew test combineReports}}
# *Incremental builds:* Any build involving test execution
h4. Related Tasks
*Upstream Tasks (Test result producers):*
* All subproject {{test}} tasks ({{{}:geode-core:test{}}},
{{{}:geode-cq:test{}}}, etc.)
* {{:geode-old-versions:test}} (explicitly problematic)
* Any custom test tasks in subprojects
*Downstream Tasks (Report consumers):*
* {{:combineReports}} (aggregates all test results)
* CI report archival tasks
* Documentation generation tasks depending on test results
h2. Benefits of This Enhancement
h3. Build Performance
* *Execution optimizations re-enabled* - Gradle can optimize scheduling
* *Better parallelization* - Task graph can be optimized for concurrent
execution
* *Incremental builds* - Up-to-date checks work correctly
* *Caching improvements* - Build cache can be used effectively
*Expected Improvements:*
* Multi-module builds: 5-10% faster scheduling
* Incremental rebuilds: Proper skipping of unchanged tasks
* CI pipelines: More efficient task execution
h3. Build Reliability
* *Deterministic ordering* - Test tasks always complete before reporting
* *No race conditions* - Explicit dependencies prevent timing issues
* *Consistent results* - Same behavior in local and CI environments
* *Predictable output* - Combined report always contains complete results
h3. Developer Experience
* *Clean build logs* - No more validation warnings
* *Clear intent* - Dependency relationships are explicit and documented
* *Easier debugging* - Task execution order is predictable
* *Better CI output* - Reduced noise in pipeline logs
h3. Code Quality
* *Best practices* - Follows Gradle recommended patterns
* *Future-proof* - Compatible with Gradle 8+ strict validation
* *Maintainable* - Explicit configuration easier to understand
* *Documented* - Clear comments explain purpose
h2.
h2. Technical Background
h3. Gradle Task Dependency Model
*Three Types of Dependencies:*
|Type|Declaration|Purpose|Example|
|Execution Order|{{dependsOn}}|Task B must run after Task
A|{{combineReports.dependsOn test}}|
|Input/Output|{{{}reportOn{}}}, {{from}}|Task B consumes Task A
output|{{reportOn testTask}}|
|Soft Order|{{mustRunAfter}}|Ordering hint, not strict|{{mustRunAfter
cleanTask}}|
*Our Solution Uses:*
* {{dependsOn}} for execution order (strict)
* {{reportOn}} for input declaration (what we consume)
h3. TestReport Task API
*Gradle's Built-in TestReport Task:*
{code:groovy}
interface TestReport {
// Declares which test tasks to aggregate
void reportOn(Test... tasks)
// Output location
File getDestinationDir()
// Inherits from Task
Task dependsOn(Object... tasks)
}
{code}
*Key Methods:*
* {{reportOn()}} - Declares test result inputs AND establishes dependency
* {{dependsOn()}} - Ensures explicit execution ordering
* Using both provides belt-and-suspenders safety
h3. Gradle 7+ Validation Changes
*What Changed in Gradle 7:*
# Stricter task input/output validation
# Detection of implicit file dependencies
# Warning when outputs consumed without declaration
# Future direction: Make warnings into errors (Gradle 8+)
*Our Compliance:*
* Explicit {{reportOn}} declares inputs
* Explicit {{dependsOn}} declares dependencies
# Gradle can validate correctness
# Optimizations can be safely enabled
----
h2. Risk Assessment
h3. Risk of NOT Fixing
*Build Performance Risk: MEDIUM*
* Continued suboptimal build performance
* Execution optimizations remain disabled
* Incremental builds less effective
* Longer CI/CD pipeline times
*Build Reliability Risk: LOW-MEDIUM*
* Potential nondeterministic execution order
* Rare race condition scenarios
* Intermittent missing test results in reports
* Harder to debug timing-related issues
*Developer Experience Risk: LOW*
* Ongoing noise in build logs
* Confusion about warning severity
* Reduced trust in build system
* More time spent investigating false alarms
h3. Risk of Fixing
*Implementation Risk: VERY LOW*
* Pure declarative configuration change
* No logic modifications
* No test code changes
* Extensive verification performed
*Compatibility Risk: NONE*
* Fully backward compatible
* No API changes
* No behavioral changes (except warning removal)
* Works with all Gradle versions
*Regression Risk: VERY LOW*
* Changes only affect build graph construction
* Does not alter test execution or reporting logic
* 17/17 CI checks passing
* Manual verification complete
*Overall Risk: LOW*
* High benefit, minimal risk
* Aligns with Gradle best practices
* Future-proofs for Gradle 8+
h2. Success Criteria
When this enhancement is implemented, the following should be true:
# *No Warnings:* Gradle validation warning eliminated from all builds
# *Optimizations Enabled:* Execution optimizations active for
{{:combineReports}}
# *Deterministic Order:* {{:geode-old-versions:test}} always executes before
{{:combineReports}}
# *Complete Report:* All module test results appear in combined report
# *No Regressions:* No new warnings or errors introduced
# *Performance:* Incremental build behavior working correctly
h2. Related Work
* Build system modernization efforts
* CI/CD pipeline optimization
* Test infrastructure improvements
* Gradle upgrade preparations (Gradle 8+)
> Security: Remove Unsafe Reflection Breaking Java Module System Encapsulation
> ----------------------------------------------------------------------------
>
> Key: GEODE-10519
> URL: https://issues.apache.org/jira/browse/GEODE-10519
> Project: Geode
> Issue Type: Improvement
> Reporter: Jinwoo Hwang
> Assignee: Jinwoo Hwang
> Priority: Major
> Fix For: 2.1.0
>
>
> h2. Summary
> UnsafeThreadLocal currently uses reflection to access private internals of
> {{java.lang.ThreadLocal}} and {{{}java.lang.ThreadLocalMap{}}}, requiring the
> JVM flag {{{}--add-opens=java.base/java.lang=ALL-UNNAMED{}}}. This creates
> maintenance burden, security concerns, and compatibility risks with future
> Java versions.
> h2. Description
> h3. Current Implementation
> The {{UnsafeThreadLocal}} class (located in
> {{{}geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java{}}})
> extends {{ThreadLocal<T>}} and provides a {{get(Thread)}} method that allows
> reading ThreadLocal values from arbitrary threads - a capability needed for
> deadlock detection.
> The current implementation uses reflection to:
> # Access the private {{getMap(Thread)}} method of ThreadLocal
> # Access the private {{getEntry(ThreadLocal)}} method of ThreadLocalMap
> # Access the private {{value}} field of ThreadLocalMap.Entry
> *Code excerpt showing reflection usage:*
> {code:java}
> private static Object get(ThreadLocal threadLocal, Thread thread) {
> try {
> Object threadLocalMap = invokePrivate(threadLocal, "getMap",
> new Class[] {Thread.class}, new Object[] {thread});
>
> if (threadLocalMap != null) {
> Object entry = invokePrivate(threadLocalMap, "getEntry",
> new Class[] {ThreadLocal.class}, new Object[] {threadLocal});
> if (entry != null) {
> return getPrivate(entry, "value");
> }
> }
> return null;
> } catch (Exception e) {
> throw new RuntimeException("Unable to extract thread local", e);
> }
> }
> {code}
> h3. Required JVM Flags
> To enable this reflection access, Geode requires the following JVM flag
> (defined in {{{}MemberJvmOptions.java{}}}):
> {code:java}
> private static final String JAVA_LANG_OPEN =
> "--add-opens=java.base/java.lang=ALL-UNNAMED";
> {code}
> This flag must be passed to every Geode member JVM, adding operational
> complexity.
> h3. Usage Context
> UnsafeThreadLocal is used in three key areas for deadlock detection:
> # *DLockService* ({{{}geode-core/.../locks/DLockService.java{}}})
> ** Tracks what distributed lock a thread is blocked on
> ** Used by: {{private final UnsafeThreadLocal<Object> blockedOn}}
> # *MessageDependencyMonitor*
> ({{{}geode-core/.../deadlock/MessageDependencyMonitor.java{}}})
> ** Tracks ReplyProcessor21 instances threads are waiting for
> ** Tracks MessageWithReply instances being processed
> ** Used by: {{private final UnsafeThreadLocal<ReplyProcessor21>
> waitingProcessors}}
> ** Used by: {{private final UnsafeThreadLocal<MessageWithReply>
> processingMessages}}
> # *DLockDependencyMonitor*
> ({{{}geode-core/.../deadlock/DLockDependencyMonitor.java{}}})
> ** Queries blockedOn status from DLockService for dependency graph
> construction
> h2. Problems with Current Approach
> h3. 1. Java Module System Violations
> *Severity: High*
> The reflection-based approach directly violates the Java Platform Module
> System (JPMS) encapsulation:
> * Accesses internal implementation details of {{java.lang}} package
> * Bypasses the module system's access controls
> * Requires explicit {{--add-opens}} directive to override module boundaries
> {*}Risk{*}: Future Java versions may further restrict or remove the ability
> to open internal packages, even with command-line flags.
> h3. 2. Maintenance and Fragility
> *Severity: High*
> The implementation is brittle and depends on:
> * Internal class names remaining unchanged (ThreadLocalMap, Entry)
> * Internal method signatures staying stable (getMap, getEntry)
> * Internal field names not changing (value)
> * Class hierarchy not being refactored
> Any changes to ThreadLocal internals in future JDK releases could break Geode
> without warning, as these are non-public APIs with no stability guarantees.
> h3. 3. Security Concerns
> *Severity: Medium*
> Using {{{}--add-opens=java.base/java.lang=ALL-UNNAMED{}}}:
> * Opens the entire {{java.lang}} package to all unnamed modules
> * Grants broader access than necessary (only need ThreadLocal access)
> * Weakens Java's security model and encapsulation guarantees
> * May trigger security audit flags in enterprise environments
> h3. 4. Operational Complexity
> *Severity: Medium*
> Current deployment requirements:
> * JVM flags must be correctly configured on all Geode servers
> * Different flags needed for Java 8 vs. Java 11+ deployments
> * Documentation and deployment scripts must maintain flag lists
> * Missing flags cause runtime failures that are difficult to diagnose
> From {{{}MemberJvmOptions.java{}}}:
> {code:java}
> static final List<String> JAVA_11_OPTIONS = Arrays.asList(
> COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
> SUN_NIO_CH_EXPORT,
> COM_SUN_MANAGEMENT_INTERNAL_OPEN,
> JAVA_LANG_OPEN, // ← Required only for UnsafeThreadLocal
> JAVA_NIO_OPEN);
> {code}
> h3. 5. Performance Overhead
> *Severity: Low*
> Reflection calls have performance costs:
> * Method lookup via reflection on each invocation
> * Access permission checks
> * Potential JIT optimization barriers
> * Exception handling overhead
> While not critical for deadlock detection (which is infrequent), it's still
> unnecessary overhead.
> h3. 6. Testing and CI/CD Complexity
> *Severity: Low*
> Test infrastructure must:
> * Configure JVM flags correctly for all test executions
> * Maintain separate flag lists for different Java versions
> * Handle flag propagation in parallel test runs
> * Document requirements for external contributors
> h2. Business Impact
> h3. Reduced Operational Risk
> Eliminating the need for {{--add-opens}} flags:
> * Simplifies deployment procedures
> * Reduces configuration errors in production
> * Removes a potential point of failure during Java upgrades
> * Improves compatibility with container orchestration platforms
> h3. Better Java Version Compatibility
> A non-reflection implementation:
> * Works seamlessly across all Java versions (8, 11, 17, 21+)
> * Doesn't depend on JDK internal implementation details
> * Reduces risk during major Java version upgrades
> * Aligns with Java's "code to interfaces, not implementations" principle
> h3. Enhanced Security Posture
> Removing reflection-based access:
> * Eliminates the need to weaken module system protections
> * Reduces attack surface by not opening internal packages
> * Simplifies security audits and compliance reviews
> * Follows principle of least privilege
> h3. Improved Developer Experience
> A cleaner implementation:
> * Easier to understand and maintain
> * No special JVM configuration needed for development
> * Simpler IDE setup and debugging
> * Reduces onboarding friction for new contributors
> h3. Long-Term Maintainability
> Moving away from internal APIs:
> * Protects against breaking changes in future JDK releases
> * Reduces technical debt
> * Makes the codebase more sustainable
> * Demonstrates modern Java best practices
> h2. Affected Components
> h3. Files Modified in This Refactoring
> *
> {{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java}}
> *
> {{geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java}}
>
> h3. Files Using UnsafeThreadLocal (Not Modified)
> These files use {{UnsafeThreadLocal}} but require no changes as the public
> API remains unchanged:
> *
> {{geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockService.java}}
> - Uses {{UnsafeThreadLocal<Object> blockedOn}} for deadlock detection
> *
> {{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/MessageDependencyMonitor.java}}
> - Uses two UnsafeThreadLocal instances for tracking message dependencies
> *
> {{geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/DLockDependencyMonitor.java}}
> - Queries UnsafeThreadLocal values for dependency graph construction
> h3. Test Files (Executed, Not Modified)
> *
> {{geode-core/src/test/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocalJUnitTest.java}}
> - All tests pass with new implementation
> h2. Scope
> * Refactoring UnsafeThreadLocal implementation to eliminate reflection
> * Updating MemberJvmOptions to remove JAVA_LANG_OPEN flag
> * Ensuring all existing tests pass with new implementation
> * Maintaining exact same public API and behavior
> * Supporting all currently supported Java versions (17, 21)
> h2. Testing Requirements
> h3. Functional Testing
> * All existing UnsafeThreadLocal unit tests must pass
> * Deadlock detection integration tests must pass
> * DLock-related distributed tests must pass
> * Message processing tests must pass
> h3. Cross-Version Testing
> * Verify functionality on Java 17, and 21
> * Ensure no regression in deadlock detection accuracy
> * Validate thread safety under concurrent access
> h3. Performance Testing
> * Verify no performance regression in deadlock detection
> * Measure overhead reduction from removing reflection
> * Test under high concurrency scenarios
> h3. Negative Testing
> * Verify proper cleanup when threads terminate
> * Test behavior with many ThreadLocal instances
> * Validate memory leak prevention
> h2. Compatibility
> h3. Backward Compatibility
> * Public API of UnsafeThreadLocal must remain unchanged
> * Behavior must be functionally equivalent to current implementation
> * No changes to deadlock detection output or timing
> h3. Forward Compatibility
> * Implementation should work with future Java versions
> * Should not depend on version-specific features
> * Should gracefully handle JVM updates
> h2. Priority Assessment
> *Recommended Priority: Medium-High*
> h3. Factors Supporting High Priority
> * Eliminates security concerns with module system violations
> * Improves long-term maintainability significantly
> * Reduces operational complexity
> * Removes technical debt that will only grow over time
> h3. Factors Supporting Medium Priority
> * Current implementation works (with JVM flags)
> * Not causing immediate production issues
> * Other reflection usage in Geode also needs addressing
> * Requires careful testing of deadlock detection
> h2. Acceptance Criteria
> # UnsafeThreadLocal no longer uses reflection to access ThreadLocal internals
> # No {{--add-opens=java.base/java.lang=ALL-UNNAMED}} flag required
> # All existing tests pass without modification
> # Deadlock detection functionality unchanged
> # Works on Java 11, 17, and 21 without special configuration
> # Performance is equal or better than current implementation
> # No memory leaks when threads are terminated
> # Code is well-documented explaining the new approach
> h2. Related Issues
> * Related to any other tickets addressing reflection usage in Geode
> * May be part of larger "Java Module System Compliance" initiative
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
