etaty / rediscala

Non-blocking, Reactive Redis driver for Scala (with Sentinel support)
Apache License 2.0
791 stars 143 forks source link

Exceptions don't seem to propagate and fail futures #131

Open duketon opened 8 years ago

duketon commented 8 years ago

I'm testing out this library @ version 1.6.0, and am integrating with JSON parsing.

I deserialize as follows:

override def deserialize(bs: ByteString) =
      Json.parse(bs.utf8String).as[T]

The .as[T] call here is unsafe and throws exceptions. When it does, the Future response from Redis does not complete and just hangs. I would expect it to return a failed Future containing the exception.

The supervisor correctly handles the exception, and the RedisClientActor reconnects:

[error] a.a.OneForOneStrategy - JsResultException(errors:List((/create,List(ValidationError(List(error.path.missing),WrappedArray())))))
...
[info] r.a.RedisClientActor - Connected to localhost/127.0.0.1:6379

However the Future hangs, uncompleted:

val redisF = redis.get[SessionRecord]("1")

redisF.onFailure {
    case t: Throwable => println(t)
}

redisF.map(_ => Ok)
  .recover {
    case NonFatal(e) => {
     println(e)
     InternalServerError
  }
}

None of these println's get executed. For quick comparison:

 val localF: Future[Int] = Future { throw new Exception("Failed to parse") }

localF.onFailure {
    case t: Throwable => println(t)
}

Prints java.lang.Exception: Failed to parse, meaning the onFailure completes with the failed Future.

Any idea what's happening here? Thanks

duketon commented 8 years ago

I found the issue in def completeSuccess(redisReply: RedisReplyT): Promise[T] in Operation:

val v = redisCommand.decodeReply(redisReply)
promise.success(v)

It should be:

val v = Try(redisCommand.decodeReply(redisReply))
promise.complete(v)

This way the Future is correctly resolved as failed with the underlying Exception.

I can work on a PR later if this is deemed as the correct solution.

etaty commented 8 years ago

Hi,

Yes you can write a PR :) Now I did not catch Exception to make sure people would not throw them.

On Mon, Mar 14, 2016 at 3:17 AM Michael Kendra notifications@github.com wrote:

I found the issue in def completeSuccess(redisReply: RedisReplyT): Promise[T] in Operation:

val v = redisCommand.decodeReply(redisReply) promise.success(v)

It should be:

val v = Try(redisCommand.decodeReply(redisReply)) promise.complete(v)

This way the Future is correctly resolved as failed with the underlying Exception.

I can work on a PR later if this is deemed as the correct solution.

— Reply to this email directly or view it on GitHub https://github.com/etaty/rediscala/issues/131#issuecomment-196119406.

carymrobbins commented 7 years ago

Is there a way to workaround this issue? How can you recover if deserialization fails?

etaty commented 7 years ago

You are deserialising with a return type of T or throw an exception. You need to send back a type holding the deserialisation error, something like Either[CustomError, T] or Either[Throwable, T]

wb14123 commented 7 years ago

I'm running into an issue that may be related to this.

On our production server, we got this error and all the redis clients seem to be failed. And we cannot catch the exception, too. It seems to be related to decode the result of hincrbyfloat:

15:11:59.708 [adhoc-data-fast-akka.actor.default-dispatcher-12] ERROR akka.actor.OneForOneStrategy - For input string: "-32113855925231610000032904145387669034530000385979199454097363604366600714474749153855751350704931223931125879 600692649128666196785408097395992482814917073799486522242283294958863621865850086997736163672348890267939103903101542744695433209648014421917696^@^@^@E�S^?^@^@^@�^B5��^?�" java.lang.NumberFormatException: For input string: "-3211385592523161000003290414538766903453000038597919945409736360436660071447474915385575135070493122393112587960069264912866619678540809739599248281491707379948652224228329495886 3621865850086997736163672348890267939103903101542744695433209648014421917696^@^@^@E�S^?^@^@^@�^B5��^?�" at sun.misc.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:2043) ~[na:1.8.0_71] at sun.misc.FloatingDecimal.parseDouble(FloatingDecimal.java:110) ~[na:1.8.0_71] at java.lang.Double.parseDouble(Double.java:538) ~[na:1.8.0_71] at redis.ByteStringDeserializerDefault$RedisDouble$.deserialize(Converter.scala:212) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.RedisCommandBulkDouble$$anonfun$decodeReply$2.apply(RedisCommand.scala:65) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.RedisCommandBulkDouble$$anonfun$decodeReply$2.apply(RedisCommand.scala:65) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at scala.Option.map(Option.scala:146) ~[org.scala-lang.scala-library-2.11.8.jar:na] at redis.RedisCommandBulkDouble$class.decodeReply(RedisCommand.scala:65) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.api.hashes.Hincrbyfloat.decodeReply(Hashes.scala:54) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.api.hashes.Hincrbyfloat.decodeReply(Hashes.scala:54) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.Operation.completeSuccess(Operation.scala:16) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.Operation$$anonfun$decodeRedisReplyThenComplete$1.apply(Operation.scala:11) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.Operation$$anonfun$decodeRedisReplyThenComplete$1.apply(Operation.scala:10) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.protocol.DecodeResult$class.foreach(RedisProtocolReply.scala:89) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.protocol.FullyDecoded.foreach(RedisProtocolReply.scala:112) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.Operation.decodeRedisReplyThenComplete(Operation.scala:10) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.actors.RedisReplyDecoder.decodeRedisReply(RedisReplyDecoder.scala:64) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.actors.RedisReplyDecoder.decodeRepliesRecur(RedisReplyDecoder.scala:50) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.actors.RedisReplyDecoder.decodeReplies(RedisReplyDecoder.scala:35) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at redis.actors.RedisReplyDecoder$$anonfun$receive$1.applyOrElse(RedisReplyDecoder.scala:28) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at akka.actor.Actor$class.aroundReceive(Actor.scala:497) ~[com.typesafe.akka.akka-actor_2.11-2.4.17.jar:na] at redis.actors.RedisReplyDecoder.aroundReceive(RedisReplyDecoder.scala:11) ~[com.github.etaty.rediscala_2.11-1.8.0.jar:1.8.0] at akka.actor.ActorCell.receiveMessage(ActorCell.scala:526) [com.typesafe.akka.akka-actor_2.11-2.4.17.jar:na] at akka.actor.ActorCell.invoke(ActorCell.scala:495) [com.typesafe.akka.akka-actor_2.11-2.4.17.jar:na] at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:257) [com.typesafe.akka.akka-actor_2.11-2.4.17.jar:na] at akka.dispatch.Mailbox.run(Mailbox.scala:224) [com.typesafe.akka.akka-actor_2.11-2.4.17.jar:na] at akka.dispatch.Mailbox.exec(Mailbox.scala:234) [com.typesafe.akka.akka-actor_2.11-2.4.17.jar:na] at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260) [org.scala-lang.scala-library-2.11.8.jar:na] at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339) [org.scala-lang.scala-library-2.11.8.jar:na] at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979) [org.scala-lang.scala-library-2.11.8.jar:na] at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107) [org.scala-lang.scala-library-2.11.8.jar:na]

How can I work around this? I add try catch to RedisDouble for now but it is surely not the best way to deal with it.

wb14123 commented 7 years ago

Hi, @duketon .What's your process of the PR? If you are busy I think I can work on this, too.