java.lang.ClassCastException: [B cannot be cast to java.lang.Long

[ichernev]

The following code produces a class cast exception

package im.imo.services.base;

import java.util.HashMap;
import java.util.Map;

import redis.clients.jedis.Jedis;
import redis.clients.jedis.Pipeline;

public class FailJedis {

    public static void main(String[] args) {
        try {
            Jedis cli = new Jedis("127.0.0.1", 17001);
            try {
                Pipeline p = cli.pipelined();
                // if you comment the next line, it works
                p.set("str-key", "val");
                Map<String, String> m = new HashMap<>();
                m.put("hash-sub-key", null);
                p.hmset("hash-key", m);
                p.sync();
            } catch (Throwable e) {
                System.out.println("good ex:" + e);
            }
            cli.smembers("set-key");
            cli.close();
        } catch (Throwable e) {
            System.out.println("bad ex:" + e);
        }
    }
}

good ex:redis.clients.jedis.exceptions.JedisDataException: value sent to redis cannot be null
bad ex:java.lang.ClassCastException: [B cannot be cast to java.util.List

What I believe is happening -- after the first exception is thrown the client is in an invalid state, which causes the following smembers command to fail with a class cast exception.

I know you shouldn't put nulls in, but it might happen (not properly sanitized client input), and the client becomes useless from that point on, even though the original exception was caught.

NOTE: I don't think this is a duplicate of https://github.com/xetorthio/jedis/pull/607 (it doesn't involve evalsha), my code fails with jedis 2.7.2.

[marcosnils]

@ichernev you need to clear the pipeline if you get an exception while using it.

Here's the updated code:

 public static void main(String[] args) {
        try {
            Jedis cli = new Jedis("127.0.0.1", 17001);
            Pipeline p = cli.pipelined();
            try {

                // if you comment the next line, it works
                p.set("str-key", "val");
                Map<String, String> m = new HashMap<String, String>();
                m.put("hash-sub-key", null);
                p.hmset("hash-key", m);
                p.sync();
            } catch (Throwable e) {
                p.clear();
                System.out.println("good ex:" + e);
            }
            cli.smembers("set-key");
            cli.close();
        } catch (Throwable e) {
            e.printStackTrace();
            System.out.println("bad ex:" + e);
        }
    }

[ichernev]

Can't you clear the pipeline automatically? Is it of any use after such an exception?

[ichernev]

There is no such method clear() on pipeline object.

[marcosnils]

@ichernev. I apologize the clear method is in the master branch (jedis 3.0). @HeartSaVioR do you know if something can be done in this scenario?

[HeartSaVioR]

@ichernev @marcosnils The issue is behind SafeEncoder.encode(). It checks null and throws JedisDataException to make sure we don't have an issue with null in protocol.

https://github.com/xetorthio/jedis/commit/0d6d37b95f503b69a703e076ff2480151a6df388#diff-4789dd51ae60fc2772902c7548f47792

Please note that it is performed when we're sending request. Pipeline sends each operations ASAP when users request, so p.set() is sent to Redis. And p.hmset() throws JedisDataException via SafeEncoder.encode(), and command isn't sent to Redis. (It is not a problem.)

Redis stores response of p.set(), but p.sync() is never called, therefore request-response relation is flawed.

In short, p.sync() should be called whether there're some errors inside Pipeline block or not. It's fixed version of your code. Please note that it is for JDK 1.6, and you can simplify the code with try-with-resource. You should take care of JedisConnectionException, since invalidated Jedis instance should be discarded ASAP.

public class NotFailJedis {

    public static void performPipelined(Jedis jedis) {
        Pipeline p = null;
        try {
            p = jedis.pipelined();
            p.set("str-key", "val");
            Map<String, String> m = new HashMap<String, String>();
            m.put("hash-sub-key", null);
            p.hmset("hash-key", m);
        } catch (JedisConnectionException e) {
            System.out.println("good ex:" + e);
            // Jedis instance is invalidated, so does Pipeline 
            p = null;

            // need to tell caller that Jedis instance is invalidated
            throw e;
        } catch (Throwable e) {
            System.out.println("good ex:" + e);

            // throw again if you want to
        } finally {
            if (p != null) {
                p.sync();
            }
        }
    }

    public static void main(String[] args) {
        Jedis cli = null;
        try {
            cli = new Jedis("127.0.0.1", 6379);
            performPipelined(cli);
            cli.smembers("set-key");
        } catch (JedisConnectionException e) {
            System.out.println("bad ex:" + e);
            cli.close();
            cli = null;
        } catch (Throwable e) {
            System.out.println("bad ex:" + e);
        } finally {
            if (cli != null) {
                cli.close();
            }
        }
    }
}

[marcosnils]

I've discussed with @HeartSaVioR about this and we agreed that making Pipeline implement Closeable from 2.7 onwards shouldn't be a big problem. I'll craft a PR soon and let u know when it's done.

[ichernev]

Making it closable is a good thing, but I would say it should be a bit more robust. Making every call to pipelined() try-catched is a bit too much, and people will forget it. I'd say that a pipeline should be synced automatically, if you execute an out-of-pipeline request, or at least have the option to do that. It can print an error if the pipeline had stuff that is not synced, but that's it. Also if you later use the pipeline for more commands it can throw exceptions.

This way is bullet proof (can't break the client irreversibly) and is easy on the developer (no boilerplate around pipelines).

[HeartSaVioR]

What if user code between initialization of Pipeline and synchronization of Pipeline which is not related to Jedis throws Exception? Jedis cannot determine it any way, and clearing its state is up to users.

Btw, Jedis is blocking sync I/O client now, so it seems not make sense that Pipeline should be synced automatically. When Pipeline can sync itself?

[ichernev]

@HeartSaVioR If the client starts using a pipeline, and then continues using the client (without pipeline), clearly he's not interested in the execution of the pipeline, so that should print an error, and sync the pipeline.

It will only be synced automatically in this weird "error" case. The other option is to make client completely useless until you sync pipeline it. It might have gone out of scope, you need to guard with try catch every single one, just so you can sync it at the end .. it doesn't make sense. Or you should guard all client ops and check for class cast and reset the connection (I'm doing this in my project, using a java Proxy).

[HeartSaVioR]

@ichernev I mean Jedis cannot determine when exception occurs on user side, and in this case try-catch statement is necessary. And ClassCastException could be hint for Jedis, but worst case it returns previous response without CCE when users request operations which have same result type. Accident could be already happened before catching.

Btw, regarding hint, Jedis can take CCE to convert JedisConnectionException so users could discard Jedis instance. It needs to be discussed more. Thanks for giving hint on this issue!

[ichernev]

Btw, regarding hint, Jedis can take CCE to convert JedisConnectionException so users could discard Jedis instance.

This will definitely help. CCE is something that doesn't really mean much outside of context, so it should be wrapped by something more meaningful. Also some documentation about which Jedis exceptions require discarding the client (so users know how to try-catch it) would be nice.

I mean Jedis cannot determine when exception occurs on user side, and in this case try-catch statement is necessary.

You're correct. But in general, jedis client being left in an unoperable state and throwing a weird exception is not good, no matter what the user did to cause it. This would be fixed by wrapping CCE (discussed in previous paragraph).

Guarding against user doing pipeline commands, then non-pipeline (without pipeline.sync) or pipeline commands / sync on a pipeline that has been invalidated by doing non-pipeline commands in between is also something that can be implemented in Jedis. In this case its a CCE that gets thrown, but if it was a well defined exception users would know to reconnect, and find the bug in their code.

[HeartSaVioR]

@ichernev Yeah, as you're already seen my comment, handling CCE doesn't cover all. But covering some will be better than covering none.

And I strongly agree that Jedis shouldn't throw CCE unless Jedis has a bug regarding type conversion. It should be wrapped to familiar Jedis Exceptions, and it notifies Jedis instance is in bad state, so JedisConnectionException would be fine.

I'll make an issue right now. Thanks again!

[github-actions[bot]]

This issue is marked stale. It will be closed in 30 days if it is not updated.