fix: handle null states in EVCacheBulkGetFuture to prevent NPE #172
Conversation
Fixes NullPointerException in bulk get operations when some operations never signal completion through callbacks. The issue was introduced in commit e4a2a9b which optimized operation state checking by collecting state during callbacks, but didn't handle cases where callbacks don't fire. This fix adds a fallback mechanism that preserves the original behavior: - Use pre-collected state when available (maintains performance optimization) - Fall back to direct operation state checking when state is null - Ensures all operations are processed, matching pre-e4a2a9bd behavior The root cause was that operationStates[i] could remain null if signalSingleOpComplete() was never called for operations that timeout, fail, or are cancelled before their completion callbacks fire.
There was a problem hiding this comment.
Pull Request Overview
Fixes a NullPointerException in EVCacheBulkGetFuture that occurs when operations never signal completion through callbacks. The fix adds fallback logic to handle null states by checking operation status directly when the optimized pre-collected state is unavailable.
- Adds null state checking with fallback to direct operation state inspection
- Preserves performance optimization by using pre-collected state when available
- Ensures all operations are processed even when callbacks don't fire
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| boolean hadTimedoutOp = false; | ||
| Operation[] opsArray = ops.toArray(new Operation[0]); |
There was a problem hiding this comment.
Converting ops to array in each method creates unnecessary overhead. Consider making opsArray a class field initialized once, or cache it to avoid repeated conversions in getSome(), handleBulkException(), and getSome() methods.
| if (!state.completed && !allCompleted) { | ||
| MemcachedConnection.opTimedOut(state.op); | ||
| hadTimedoutOp = true; | ||
| Operation op = opsArray[i]; |
There was a problem hiding this comment.
The same pattern of 'if (state == null)' fallback logic is duplicated across multiple methods. Consider extracting this logic into a private helper method to reduce code duplication and improve maintainability.
|
|
||
| if (state == null) { | ||
| // Operation never signaled completion, fall back to direct checking | ||
| if (op.getState() != OperationState.COMPLETE) { |
There was a problem hiding this comment.
Isn't this the same as (the reason why) state == null? Do we ever go into line 138? (same for line 259)
Fixes NullPointerException in bulk get operations when some operations
never signal completion through callbacks. The issue was introduced in
commit e4a2a9b which optimized operation state checking by collecting
state during callbacks, but didn't handle cases where callbacks don't fire.
This fix adds a fallback mechanism that preserves the original behavior:
The root cause was that operationStates[i] could remain null if
signalSingleOpComplete() was never called for operations that timeout,
fail, or are cancelled before their completion callbacks fire.