Skip to content

fix: handle null states in EVCacheBulkGetFuture to prevent NPE #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pkarumanchi9 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: handle null states in EVCacheBulkGetFuture to prevent NPE #172

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 95 additions & 22 deletions evcache-core/src/main/java/com/netflix/evcache/operation/EVCacheBulkGetFuture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,25 +120,55 @@ public Map<String, T> getSome(long to, TimeUnit unit, boolean throwException, bo
}

boolean hadTimedoutOp = false;
Operation[] opsArray = ops.toArray(new Operation[0]);
Copy link

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
for (int i = 0; i < operationStates.length(); i++) {
SingleOperationState state = operationStates.get(i);
if (!state.completed && !allCompleted) {
MemcachedConnection.opTimedOut(state.op);
hadTimedoutOp = true;
Operation op = opsArray[i];
Copy link

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

if (state == null) {
// Operation never signaled completion, fall back to direct checking
if (op.getState() != OperationState.COMPLETE) {
Copy link
Contributor

@shy-1234 shy-1234 Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as (the reason why) state == null? Do we ever go into line 138? (same for line 259)

if (!allCompleted) {
MemcachedConnection.opTimedOut(op);
hadTimedoutOp = true;
} else {
MemcachedConnection.opSucceeded(op);
}
} else {
MemcachedConnection.opSucceeded(op);
}
} else {
MemcachedConnection.opSucceeded(state.op);
// Use pre-collected state for performance
if (!state.completed && !allCompleted) {
MemcachedConnection.opTimedOut(state.op);
hadTimedoutOp = true;
} else {
MemcachedConnection.opSucceeded(state.op);
}
}
}

if (!allCompleted && !hasZF && hadTimedoutOp) statusString = EVCacheMetricsFactory.TIMEOUT;

for (int i = 0; i < operationStates.length(); i++) {
SingleOperationState state = operationStates.get(i);
if (state.cancelled) {
if (hasZF) statusString = EVCacheMetricsFactory.CANCELLED;
if (throwException) throw new ExecutionException(new CancellationException("Cancelled"));
Operation op = opsArray[i];

if (state == null) {
// Fall back to direct operation checking
if (op.isCancelled()) {
if (hasZF) statusString = EVCacheMetricsFactory.CANCELLED;
if (throwException) throw new ExecutionException(new CancellationException("Cancelled"));
}
if (op.hasErrored() && throwException) throw new ExecutionException(op.getException());
} else {
// Use pre-collected state
if (state.cancelled) {
if (hasZF) statusString = EVCacheMetricsFactory.CANCELLED;
if (throwException) throw new ExecutionException(new CancellationException("Cancelled"));
}
if (state.errored && throwException) throw new ExecutionException(state.op.getException());
}
if (state.errored && throwException) throw new ExecutionException(state.op.getException());
}

Map<String, T> m = new HashMap<String, T>();
Expand Down Expand Up @@ -219,20 +249,41 @@ public CompletableFuture<Map<String, T>> getAsyncSome(long timeout, TimeUnit uni

public void handleBulkException() {
ExecutionException t = null;
Operation[] opsArray = ops.toArray(new Operation[0]);
for (int i = 0; i < operationStates.length(); i++) {
SingleOperationState state = operationStates.get(i);
if (!state.completed) {
if (state.cancelled) {
throw new RuntimeException(new ExecutionException(new CancellationException("Cancelled")));
} else if (state.errored) {
throw new RuntimeException(new ExecutionException(state.op.getException()));
Operation op = opsArray[i];

if (state == null) {
// Fall back to direct operation checking
if (op.getState() != OperationState.COMPLETE) {
if (op.isCancelled()) {
throw new RuntimeException(new ExecutionException(new CancellationException("Cancelled")));
} else if (op.hasErrored()) {
throw new RuntimeException(new ExecutionException(op.getException()));
} else {
op.timeOut();
MemcachedConnection.opTimedOut(op);
t = new ExecutionException(new CheckedOperationTimeoutException("Checked Operation timed out.", op));
}
} else {
state.op.timeOut();
MemcachedConnection.opTimedOut(state.op);
t = new ExecutionException(new CheckedOperationTimeoutException("Checked Operation timed out.", state.op));
MemcachedConnection.opSucceeded(op);
}
} else {
MemcachedConnection.opSucceeded(state.op);
// Use pre-collected state
if (!state.completed) {
if (state.cancelled) {
throw new RuntimeException(new ExecutionException(new CancellationException("Cancelled")));
} else if (state.errored) {
throw new RuntimeException(new ExecutionException(state.op.getException()));
} else {
state.op.timeOut();
MemcachedConnection.opTimedOut(state.op);
t = new ExecutionException(new CheckedOperationTimeoutException("Checked Operation timed out.", state.op));
}
} else {
MemcachedConnection.opSucceeded(state.op);
}
}
}

Expand All @@ -257,19 +308,41 @@ public void doAsyncGetSome(CompletableFuture<Map<String, T>> promise) {
public Single<Map<String, T>> getSome(long to, TimeUnit units, boolean throwException, boolean hasZF, Scheduler scheduler) {
return observe().timeout(to, units, Single.create(subscriber -> {
try {
Operation[] opsArray = ops.toArray(new Operation[0]);
for (int i = 0; i < operationStates.length(); i++) {
SingleOperationState state = operationStates.get(i);
if (!state.completed) {
MemcachedConnection.opTimedOut(state.op);
Operation op = opsArray[i];

if (state == null) {
// Fall back to direct operation checking
if (op.getState() != OperationState.COMPLETE) {
MemcachedConnection.opTimedOut(op);
} else {
MemcachedConnection.opSucceeded(op);
}
} else {
MemcachedConnection.opSucceeded(state.op);
// Use pre-collected state
if (!state.completed) {
MemcachedConnection.opTimedOut(state.op);
} else {
MemcachedConnection.opSucceeded(state.op);
}
}
}

for (int i = 0; i < operationStates.length(); i++) {
SingleOperationState state = operationStates.get(i);
if (state.cancelled && throwException) throw new ExecutionException(new CancellationException("Cancelled"));
if (state.errored && throwException) throw new ExecutionException(state.op.getException());
Operation op = opsArray[i];

if (state == null) {
// Fall back to direct operation checking
if (op.isCancelled() && throwException) throw new ExecutionException(new CancellationException("Cancelled"));
if (op.hasErrored() && throwException) throw new ExecutionException(op.getException());
} else {
// Use pre-collected state
if (state.cancelled && throwException) throw new ExecutionException(new CancellationException("Cancelled"));
if (state.errored && throwException) throw new ExecutionException(state.op.getException());
}
}

Map<String, T> m = new HashMap<String, T>();
Expand Down