debasishg / scala-redis

A scala library for connecting to a redis server, or a cluster of redis nodes using consistent hashing on the client side.
1.02k stars 219 forks source link

"Exception: ERR wrong number of arguments f or 'expire' command" occurs when attempting to expire a key that contains spaces. #286

Closed noahlz closed 2 years ago

noahlz commented 2 years ago

using v3.41 (thanks!) I encounter the following error when attempting to expire a key that has spaces in it.

Debugging:

DEB [20211029-17:34:09.856] [pool-25-thread-7] c.r.RedisClient C: LPUSH 617c68cc27ebbf55984a7271:c349p7127:3:Not Available:Not Available:GBP_GBP_L_CEQ:GBP_GBP_L_CEQ [B@3b26919e\r\nEXPIRE 617c68cc27ebbf55984a7271:c349p7127:3:Not Available:Not Available:GBP_GBP_L_CEQ:GBP_GBP_L_CEQ 1800\r\nLPUSH 617c68cc27ebbf55984a7271:c349p7127:3:Italy:N/A:N/A:N/A [B@680e412d\r\nEXPIRE 617c68cc27ebbf55984a7271:c349p7127:3:Italy:N/A:N/A:N/A 1800\r\nLPUSH 617c68cc27ebbf55984a7271:c349p7127:3:United States:Consumer Discretionary:Citi Trends Inc:Citi Trends Inc 

It seems that the LPUSH succeeds but it chokes on the EXPIRE command. Note EXPIRE 617c68cc27ebbf55984a7271:c349p7127:3:Not Available:Not Available:GBP_GBP_L_CEQ:GBP_GBP_L_CEQ 1800

I'm looking into this further...

noahlz commented 2 years ago

This seems to be the issue: the implementation of batch commands

 38   def send[A](command: String, args: Seq[Any])(result: => A)(implicit format: Format): A = try {
 39     if (batch == BATCH) {
 40       handlers :+= ((command, () => result))
 41       commandBuffer.append((List(command) ++ args.toList).mkString(" ") ++ crlf)
 42       null.asInstanceOf[A] // hack

Note line 41 - it takes all arguments and concatenates them "as-is" to the Redis.

Do we want the user to be required to escape their key with quotes, or do that automatically? Given this function is completely agnostic as to what args are, seems like it's up to the user(...?) Unless there are quotes added to the key of functions where appropriate, i.e. RedisClient.expire

https://code.google.com/archive/p/redis/issues/197

noahlz commented 2 years ago

@debasishg Ok, from reading the code, it's rather clear to me that we should be providing double quotes around keys (and all arguments, really) when using batchedPipeline. The only question I have is - do we want to do "magic double quotes" (apologies to PHP devs from the early 00's reading this), or merely document for users how to use batchedPipeline properly, i.e. quote your keys, etc.

noahlz commented 2 years ago

Ok, I added quotes to the keys, and it got me past EXPIRE, but now LPUSH seems to be having issues as well

First we

LPUSH "617c85850d04fc7365744c2f:c349p7127:3:Not Available:Not Available:JPY_JPY_L_CEQ" [B@2e14997d [B@7a32edf6 [B@38fe1e9f [B@1fd4a327

Which is supposed to be storing Kryo serialized Objects

Then we try to read them back with zrange, it's clear that it serialized the string representation... not the raw bytes

dev-redis.kube.novus.local:6379> ZRANGE 617c85850d04fc7365744c2f:c349p7127:PIT:3 0 -1
   1) "[B@104dd5c2"
   2) "[B@106e5566"
   3) "[B@107aef31"
   4) "[B@107e3da6"
   5) "[B@10b9387f"
   6) "[B@10d3c559"
   7) "[B@10d3f7c2"
   8) "[B@10f3b456"
   9) "[B@113c9a94"
noahlz commented 2 years ago

I am able to partially solve this by encoding our byte array data as Base64 strings. Not exactly an ideal solution, as it increases the size of our data, and our code is now more complicated having to go Object > Bytes (via Kryo serialization) > Base64 String > Redis and then back again.

If we can't update the batchedPipeline implementation to more robustly handle Array[Binary] command arguments for LPUSH and such - what is the most effective way to story binary java objects via that function (if not Base64?).

debasishg commented 2 years ago

Hi Noah - the commands in batchedPipeline should be exactly the same as you do for normal ones. In case u need to pass something or receive something that needs custom serialization/ deserialization, you need to pass in custom Format and Parse instances. Check out the tests that deal with serialization. I will have some examples as well later today that deal with serialization.

Thanks.

On Sat, 30 Oct 2021 at 5:21 AM, Noah Zucker @.***> wrote:

Ok, I added quotes to the keys, and it got me past EXPIRE, but now LPUSH seems to be having issues as well

First we

LPUSH "617c85850d04fc7365744c2f:c349p7127:3:Not Available:Not Available:JPY_JPY_L_CEQ" @. @. @. @.

Which is storing Kryo serialized Objects

Then we try to read them back with zrange, it's claar that it serialized the string representation... not the raw bytes

dev-redis.kube.novus.local:6379> ZRANGE 617c85850d04fc7365744c2f:c349p7127:PIT:3 0 -1 1) @." 2) @." 3) @." 4) @." 5) @." 6) @." 7) @." 8) @." 9) @.***"

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/debasishg/scala-redis/issues/286#issuecomment-955104728, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2FX6QGZMNHDTW3ETYBTDUJMXQRANCNFSM5HAHBJYQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Sent from my iPhone

debasishg commented 2 years ago

@noahlz hmm .. Looks like the Format and Parse are not passed properly when sending commands in batch. I will work on this very soon and get back to you.

BTW thanks for the other PR - I merged it.

debasishg commented 2 years ago

I added a PR .. https://github.com/debasishg/scala-redis/pull/288 .. will u able to test with a local build ? Otherwise I can make one more release .. Please take a look at the tests in PipelineSpec.

debasishg commented 2 years ago

@noahlz - In case u face any issue with the above PR can u pls send me a reproducible test case here which I can work through.

noahlz commented 2 years ago

@debasishg I opened a PR #290 demonstrating some cases where batchedPipeline errors on keys containing spaces or single quotes.

More interesting:

I ran our application with some data sets that don't have spaces or single-quotes in keys. There was no error, but after we lpush a number of Array[Binary] elements the application threads were hanging on this stack:

"pool-25-thread-5" #614 prio=5 os_prio=0 tid=0x00007f6cc0987000 nid=0x29c6 runnable [0x00007f6bdee35000]
   java.lang.Thread.State: RUNNABLE
        at java.net.SocketInputStream.socketRead0(Native Method)
        at java.net.SocketInputStream.socketRead(SocketInputStream.java:116)
        at java.net.SocketInputStream.read(SocketInputStream.java:171)
        at java.net.SocketInputStream.read(SocketInputStream.java:141)
        at java.io.BufferedInputStream.fill(BufferedInputStream.java:246)
        at java.io.BufferedInputStream.read(BufferedInputStream.java:265)
        - locked <0x00000007155e0aa8> (a java.io.BufferedInputStream)
        at com.redis.IO.readLine(IO.scala:98)
        at com.redis.IO.readLine$(IO.scala:91)
        at com.redis.Redis.readLine(RedisClient.scala:34)
        at com.redis.Reply.receive(RedisProtocol.scala:136)
        at com.redis.Reply.receive$(RedisProtocol.scala:136)
        at com.redis.Redis.receive(RedisClient.scala:34)
        at com.redis.R.asLong(RedisProtocol.scala:251)
        at com.redis.R.asLong$(RedisProtocol.scala:251)
        at com.redis.Redis.asLong(RedisClient.scala:34)
        at com.redis.ListOperations.$anonfun$lpush$1(ListOperations.scala:10)
        at com.redis.ListOperations$$Lambda$7779/1438318932.apply(Unknown Source)
        at com.redis.RedisClient.$anonfun$batchedPipeline$4(RedisClient.scala:250)
        at com.redis.RedisClient$$Lambda$7755/1478899735.apply(Unknown Source)
        at scala.collection.TraversableLike.$anonfun$map$1(TraversableLike.scala:273)
        at scala.collection.TraversableLike$$Lambda$4295/1723452362.apply(Unknown Source)
        at scala.collection.Iterator.foreach(Iterator.scala:943)
        at scala.collection.Iterator.foreach$(Iterator.scala:943)
        at scala.collection.AbstractIterator.foreach(Iterator.scala:1431)
        at scala.collection.IterableLike.foreach(IterableLike.scala:74)
        at scala.collection.IterableLike.foreach$(IterableLike.scala:73)
        at scala.collection.AbstractIterable.foreach(Iterable.scala:56)
        at scala.collection.TraversableLike.map(TraversableLike.scala:273)
        at scala.collection.TraversableLike.map$(TraversableLike.scala:266)
        at scala.collection.AbstractTraversable.map(Traversable.scala:108)
        at com.redis.RedisClient.$anonfun$batchedPipeline$2(RedisClient.scala:250)
        at com.redis.RedisClient$$Lambda$7725/99231071.apply(Unknown Source)
        at com.redis.Redis.send(RedisClient.scala:90)
        at com.redis.Redis.send(RedisClient.scala:93)
        at com.redis.Redis.send(RedisClient.scala:93)
        at com.redis.Redis.send(RedisClient.scala:93)
        at com.redis.Redis.send(RedisClient.scala:93)
        at com.redis.Redis.send(RedisClient.scala:93)
        at com.redis.RedisClient.batchedPipeline(RedisClient.scala:250)

Should I open a different issue for this? This might be more difficult to provide a test case.

noahlz commented 2 years ago

Can confirm this issue is resolved in the latest changes to #288! Many thanks!

debasishg commented 2 years ago

@noahlz Thanks .. closing this issue then.