JinwooHwang opened a new pull request, #7955:
URL: https://github.com/apache/geode/pull/7955
## Summary
This PR removes Apache Geode's dependency on the internal
`sun.nio.ch.DirectBuffer` interface, eliminating the need for the
`--add-exports=java.base/sun.nio.ch=ALL-UNNAMED` JVM flag. The refactoring uses
only public Java APIs while maintaining all existing functionality for buffer
slice tracking in the BufferPool.
## Motivation
### Problem
Apache Geode previously accessed the internal `sun.nio.ch.DirectBuffer`
interface to track buffer slice-to-original mappings in `BufferPool`. This
approach had several critical issues:
1. **Security Risk**: Required exporting internal NIO channel implementation
to all unnamed modules
2. **Module System Violation**: Bypassed Java Platform Module System (JPMS)
encapsulation
3. **Maintenance Burden**: Depended on non-public APIs that could change
without notice
4. **Operational Complexity**: Required JVM export flags on all Geode server
instances
5. **Compliance Issues**: Triggered security audit flags in enterprise
environments
### Impact
- One less JVM flag to configure (`SUN_NIO_CH_EXPORT`)
- Improved security posture by not exposing internal I/O implementation
- Better forward compatibility with future Java versions
- Simpler deployment and operations
- Reduced technical debt
## Changes
### New Component: BufferAttachmentTracker
Created `BufferAttachmentTracker` class in `geode-core` to replace internal
API usage:
**Key Features:**
- Uses `WeakHashMap<ByteBuffer, ByteBuffer>` for slice-to-original mapping
- Thread-safe with synchronized access
- Automatic memory cleanup via weak references
- Simple public API: `recordSlice()` and `getOriginal()`
**Implementation Details:**
```java
public class BufferAttachmentTracker {
private static final Map<ByteBuffer, ByteBuffer> sliceToOriginal =
Collections.synchronizedMap(new WeakHashMap<>());
public static void recordSlice(ByteBuffer slice, ByteBuffer original) {
sliceToOriginal.put(slice, original);
}
public static ByteBuffer getOriginal(ByteBuffer buffer) {
return sliceToOriginal.getOrDefault(buffer, buffer);
}
}
```
**Why WeakHashMap:**
- Allows garbage collection of unused buffer mappings
- Prevents memory leaks when buffers are no longer referenced
- No manual cleanup required
- Follows Java best practices for cache-like structures
### Modified: BufferPool.java
Updated buffer slice creation and retrieval:
**Before (using internal API):**
```java
import org.apache.geode.unsafe.internal.sun.nio.ch.DirectBuffer;
ByteBuffer getPoolableBuffer(ByteBuffer buffer) {
if (buffer instanceof DirectBuffer) {
ByteBuffer attachment = ((DirectBuffer) buffer).attachment();
if (attachment != null) {
return attachment;
}
}
return buffer;
}
```
**After (using public API):**
```java
ByteBuffer acquireDirectBuffer(int size, boolean send) {
// ... buffer allocation code ...
if (result.capacity() > size) {
ByteBuffer original = result;
result.position(0).limit(size);
result = result.slice();
// Track the slice-to-original mapping
BufferAttachmentTracker.recordSlice(result, original);
}
return result;
}
@VisibleForTesting
ByteBuffer getPoolableBuffer(final ByteBuffer buffer) {
return BufferAttachmentTracker.getOriginal(buffer);
}
```
**Key Changes:**
- Record slice mapping when buffer is sliced (line 125)
- Simplified `getPoolableBuffer()` to use tracker
- No dependency on internal APIs
- Maintains exact same functionality
### Removed: DirectBuffer Wrapper
Deleted internal API wrapper from `geode-unsafe` module:
- `geode-unsafe/src/main/java/.../DirectBuffer.java` (36 lines)
- `geode-unsafe/src/test/java/.../DirectBufferTest.java` (53 lines)
These files are no longer needed as we don't access internal APIs.
### Updated: MemberJvmOptions.java
Removed `SUN_NIO_CH_EXPORT` from required JVM options:
**Before:**
```java
private static final String SUN_NIO_CH_EXPORT =
"--add-exports=java.base/sun.nio.ch=ALL-UNNAMED";
static final List<String> JAVA_11_OPTIONS = Arrays.asList(
COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
SUN_NIO_CH_EXPORT, // Now removed
COM_SUN_MANAGEMENT_INTERNAL_OPEN,
JAVA_LANG_OPEN,
JAVA_NIO_OPEN);
```
**After:**
```java
// SUN_NIO_CH_EXPORT removed - no longer needed
static final List<String> JAVA_11_OPTIONS = Arrays.asList(
COM_SUN_JMX_REMOTE_SECURITY_EXPORT,
COM_SUN_MANAGEMENT_INTERNAL_OPEN,
JAVA_LANG_OPEN,
JAVA_NIO_OPEN);
```
### New Tests: BufferAttachmentTrackerTest.java
Comprehensive test coverage (130 lines) includes:
**Test Cases:**
- `testRecordAndRetrieveSlice()` - Basic slice tracking
- `testMultipleSlicesFromSameOriginal()` - Multiple slices share original
- `testNestedSlices()` - Slice of slice handling
- `testNonSlicedBufferReturnsItself()` - Identity for non-sliced buffers
- `testWeakReferenceCleanup()` - Memory cleanup via GC
- `testThreadSafety()` - Concurrent access from multiple threads
- `testNullHandling()` - Null safety
- `testMixedBufferTypes()` - Direct and heap buffers
**Thread Safety Test:**
```java
@Test
public void testThreadSafety() throws Exception {
ExecutorService executor = Executors.newFixedThreadPool(10);
List<Future<?>> futures = new ArrayList<>();
for (int i = 0; i < 100; i++) {
futures.add(executor.submit(() -> {
ByteBuffer original = ByteBuffer.allocateDirect(1024);
ByteBuffer slice = original.slice();
BufferAttachmentTracker.recordSlice(slice, original);
assertThat(BufferAttachmentTracker.getOriginal(slice))
.isSameAs(original);
}));
}
// Verify all completed successfully
for (Future<?> future : futures) {
future.get(5, TimeUnit.SECONDS);
}
}
```
## Testing
### Existing Tests
- All `BufferPool` tests pass without modification
- No functional changes to buffer pooling behavior
- Existing integration tests validate end-to-end functionality
### New Tests
- `BufferAttachmentTrackerTest` provides comprehensive coverage
- Tests validate thread safety, memory cleanup, and edge cases
- Performance characteristics verified with concurrent access patterns
## Performance Impact
**Expected:** Negligible to none
**Analysis:**
- `WeakHashMap` lookup: O(1) average case, same as internal `attachment()`
field access
- Synchronization overhead minimal (buffer slice creation is infrequent
relative to I/O)
- Memory overhead: One WeakHashMap entry per slice (small fixed cost)
- GC impact: Weak references cleaned up automatically, no manual management
**Comparison:**
| Aspect | Old (DirectBuffer) | New (BufferAttachmentTracker) |
|--------|-------------------|------------------------------|
| Lookup | Field access | HashMap lookup |
| Thread Safety | Not applicable | Synchronized |
| Memory Cleanup | Manual | Automatic (WeakHashMap) |
| API Stability | Internal (unstable) | Public (stable) |
## Compatibility
### Backward Compatibility
- **Fully backward compatible** - no API changes to BufferPool
- Existing code continues to work without modification
- No changes to buffer pooling behavior or semantics
### Forward Compatibility
- Works on Java 17, 21, and future versions
- No dependency on JDK internals that could change
- Aligns with Java module system best practices
## Security Improvements
1. **Reduced Attack Surface**: No longer exposes internal NIO channel
implementation
2. **Principle of Least Privilege**: Uses only necessary public APIs
3. **Module Encapsulation**: Respects JPMS boundaries
4. **Audit Compliance**: Eliminates security audit flags for internal API
usage
## Related Work
This PR is part of a broader initiative to eliminate all internal API usage
in Apache Geode:
- **GEODE-10519**: Eliminated `UnsafeThreadLocal` access to `java.lang`
internals (completed)
- **GEODE-10520**: This PR - Eliminate `DirectBuffer` access to `sun.nio.ch`
internals
- **Future**: AddressableMemoryManager access to `java.nio` internals
- **Future**: VMStats50 access to `com.sun.management.internal` internals
**Goal**: Make Apache Geode fully compliant with Java Platform Module
System, eliminating all `--add-opens` and `--add-exports` requirements.
## Documentation Updates
- Added comprehensive Javadoc to `BufferAttachmentTracker`
- Updated `BufferPool.getPoolableBuffer()` documentation
- Added inline comments explaining the tracking mechanism
## Checklist
- [x] Code follows Apache Geode coding standards
- [x] All new code has comprehensive test coverage
- [x] All existing tests pass without modification
- [x] No breaking changes to public APIs
- [x] Internal API usage eliminated
- [x] Thread safety validated
- [x] Memory management verified (no leaks)
- [x] Documentation added for new components
- [x] JVM flag removed from MemberJvmOptions
- [x] Backward compatibility maintained
## Files Changed
**Summary**: 6 files changed, 224 insertions(+), 111 deletions(-)
### Added
-
`geode-core/src/main/java/org/apache/geode/internal/net/BufferAttachmentTracker.java`
(89 lines)
- New tracker class using WeakHashMap for slice-to-original mappings
-
`geode-core/src/test/java/org/apache/geode/internal/net/BufferAttachmentTrackerTest.java`
(130 lines)
- Comprehensive test suite for tracker functionality
### Modified
- `geode-core/src/main/java/org/apache/geode/internal/net/BufferPool.java`
(20 lines changed)
- Updated to use BufferAttachmentTracker instead of DirectBuffer
-
`geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java`
(7 lines deleted)
- Removed SUN_NIO_CH_EXPORT constant and usage
### Deleted
-
`geode-unsafe/src/main/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBuffer.java`
(36 lines)
- Internal API wrapper no longer needed
-
`geode-unsafe/src/test/java/org/apache/geode/unsafe/internal/sun/nio/ch/DirectBufferTest.java`
(53 lines)
- Tests for removed wrapper
## Review Focus Areas
1. **Thread Safety**: Review synchronized access in `BufferAttachmentTracker`
2. **Memory Management**: Verify WeakHashMap usage prevents memory leaks
3. **Test Coverage**: Ensure test cases cover all edge cases
4. **Performance**: Validate no significant overhead from HashMap lookups
5. **API Compatibility**: Confirm no breaking changes to BufferPool API
## Additional Notes
### Why Not Other Approaches?
**Alternative 1: Store attachment in custom field**
- Requires modifying ByteBuffer (not possible - final class)
- Would need wrapper class (defeats purpose of removing wrappers)
**Alternative 2: IdentityHashMap**
- Considered but rejected
- No automatic cleanup (requires manual memory management)
- Risk of memory leaks if cleanup missed
**Alternative 3: Keep DirectBuffer wrapper**
- Rejected - doesn't solve the core problem
- Still requires JVM export flags
- Still depends on unstable internal APIs
**Chosen: WeakHashMap**
- Automatic cleanup via garbage collection
- Thread-safe with synchronization
- Standard Java practice for cache-like structures
- No risk of memory leaks
### Future Enhancements
Potential improvements for future consideration:
1. Monitor WeakHashMap size for metrics/debugging
2. Add configurable cleanup thresholds if needed
3. Consider ConcurrentHashMap with weak values if synchronization becomes
bottleneck
However, current implementation is sufficient for production use.
## References
- **JIRA**: GEODE-10520
- **Related PR**: GEODE-10519 (UnsafeThreadLocal refactoring)
- **Documentation**: [Java Platform Module
System](https://openjdk.org/jeps/261)
- **Best Practice**: [Effective Java - Item 7: Eliminate obsolete object
references](https://www.oreilly.com/library/view/effective-java/9780134686097/)
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline review of your contribution we ask that you
ensure you've taken the following steps. -->
### For all changes, please confirm:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in
the commit message?
- [x] Has your PR been rebased against the latest commit within the target
branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies
licensed in a way that is compatible for inclusion under [ASF
2.0](http://www.apache.org/legal/resolved.html#category-a)?
--
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]
