Fix NPE when manually flushing a batch

[luciopaiva]

When manually flushing a batch with a set size, we can potentially run into a NPE if the command queue is empty when flush() is called. This PR is fixing that.

I ran into this problem while working on a code that sends commands to Redis in batches. In my code I defined an interface like this:

@BatchSize(100)
public interface MyCommands extends Commands, BatchExecutor {

    public void set(String key, String value);
}

For each batch, no matter how many commands it has, I want to make sure that it flushes the commands as soon as I finish enqueuing them, like this:

for (int i = 0; i < myBatchSize; i++) {
    api.set(key[i], value[i]);
}
api.flush();

And that's when I ran into a problem: when myBatchSize == 100, after the 100th is enqueued a flush is automatically triggered because of @BatchSize(100), and the queue gets emptied. Right after, api.flush() is called and Lettuce raises a NPE.

A simpler way to reproduce the problem is to just call flush() without ever enqueueing anything, so that's what I did in the test that I wrote.

The fix is really simple and just involves replacing a do..while with a while block that first checks if the queue is empty before trying to poll from it.

The bug is there since at least 5.2 (the earliest version I tested).

---

Make sure that:

• You have read the contribution guidelines.
• You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.

[luciopaiva]

I am removing this assert because it doesn't help with production code. I searched for uses of assert throughout the code and found only this and another one a few lines below, which I'm also removing.

[luciopaiva]

The code here in prepareDefaultFlush() is already checking if the queue is empty before poll()ing from it, so I didn't have to do anything.

[mp911de]

Care to provide the stack trace showing where the exception originates from?

[luciopaiva]

Care to provide the stack trace showing where the exception originates from?

Sure. This is 6.3.0.BUILD-SNAPSHOT when I run the new test without the fix:

java.lang.NullPointerException
	at io.lettuce.core.StatefulRedisConnectionImpl.preProcessCommand(StatefulRedisConnectionImpl.java:201)
	at io.lettuce.core.StatefulRedisConnectionImpl.dispatch(StatefulRedisConnectionImpl.java:159)
	at io.lettuce.core.dynamic.SimpleBatcher.doFlush(SimpleBatcher.java:138)
	at io.lettuce.core.dynamic.SimpleBatcher.flush(SimpleBatcher.java:108)
	at io.lettuce.core.dynamic.SimpleBatcher.flush(SimpleBatcher.java:85)
	at io.lettuce.core.dynamic.BatchExecutableCommandLookupStrategy$1.execute(BatchExecutableCommandLookupStrategy.java:79)
	at io.lettuce.core.dynamic.RedisCommandFactory$CommandFactoryExecutorMethodInterceptor.invoke(RedisCommandFactory.java:235)
	at io.lettuce.core.dynamic.intercept.MethodInterceptorChain$MethodInterceptorContext.proceed(MethodInterceptorChain.java:118)
	at io.lettuce.core.dynamic.intercept.MethodInterceptorChain$PooledMethodInvocation.proceed(MethodInterceptorChain.java:201)
	at io.lettuce.core.dynamic.intercept.DefaultMethodInvokingInterceptor.invoke(DefaultMethodInvokingInterceptor.java:45)
	at io.lettuce.core.dynamic.intercept.MethodInterceptorChain$MethodInterceptorContext.proceed(MethodInterceptorChain.java:118)
	at io.lettuce.core.dynamic.intercept.MethodInterceptorChain.invoke(MethodInterceptorChain.java:80)
	at io.lettuce.core.dynamic.intercept.InvocationProxyFactory$InterceptorChainInvocationHandler.handleInvocation(InvocationProxyFactory.java:102)
	at io.lettuce.core.internal.AbstractInvocationHandler.invoke(AbstractInvocationHandler.java:80)
	at io.lettuce.core.dynamic.$Proxy47.flush(Unknown Source)
	at io.lettuce.core.dynamic.RedisCommandsBatchingIntegrationTests.shouldNotCrashWhenFlushCalledWithEmptyQueue(RedisCommandsBatchingIntegrationTests.java:110)

SimpleBatcher::doFlush() calls connection.dispatch() passing it a list with a null value, and then it eventually crashes inside StatefulRedisConnectionImpl::preProcessCommand() when it tries to read the null value as a command.

[mp911de]

Thank you for your contribution. That's merged, polished, and backported now.

[luciopaiva]

@mp911de how is this being backported? Will we see this fix in 5.x, 4.x, etc at some point?

[mp911de]

It is backported to Lettuce 6.2.5, which I released today. I maintain only a single bugfix branch at a time.