ReactiveCouchbase / ReactiveCouchbase-core

Core library for ReactiveCouchbase
Apache License 2.0
64 stars 28 forks source link

Exception when trying to get non existing document #27

Closed nielsboldt closed 10 years ago

nielsboldt commented 10 years ago

When running with failfutures=true the following test fails. Also I'm running with version 0.2

..

"lookup without throwing" in {

  val none : Option[Cookie] = bucket.get[Cookie]("a dummy that doesn't exists")
  none must beNone

}

.

with the trace

[error] OperationFailedException: : Operation failed : Not found (CouchbaseFutures.scala:141) [error] org.reactivecouchbase.client.CouchbaseFutures$.org$reactivecouchbase$client$CouchbeseFutures$$complete$3(CouchbaseFutures.scala:141) [error] org.reactivecouchbase.client.CouchbaseFutures$$anon$2.onComplete(CouchbaseFutures.scala:157) [error] org.reactivecouchbase.client.CouchbaseFutures$$anon$2.onComplete(CouchbaseFutures.scala:156) [error] net.spy.memcached.internal.AbstractListenableFuture$1.run(AbstractListenableFuture.java:117) [error] akka.dispatch.TaskInvocation.run(AbstractDispatcher.scala:42) [error] akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(AbstractDispatcher.scala:386)

Is that the intended behaviour?

mathieuancelin commented 10 years ago

I rewrote your here

"lookup without throwing" in {
      val none : Future[Option[JsValue]] = bucket.get[JsValue]("a dummy that doesn't exists")
      none.onComplete {
        case Success(Some(stuff)) => println(s"Some $stuff" )
        case Success(None) => println(s"None, it doesn't exists")
        case Failure(e) => println(s"Got some error : ${e.getMessage}")
      }

      Await.result(none.recover {
        case _ => None
      }, timeout) must beNone
    }

Here I just add a callback (onComplete) to print the state of the future. If you run that test with the default config of ReactiveCouchbase (with failfutures=false), then the test will pass and display None, it doesn't exists because, by default the Futures returned by ReactiveCouchbase are purely technical and are not affected by 'user failure', that's why the get operation return an Option. So it mean that these Futures will never fail except in case of a big technical issue (Couchbase server unreachable, timeout, wrong view, etc ...)

However, if you set failfutures=true then you specifically say to ReactiveCouchbase : "If the underlying Couchbase operation fails (ie. the OperationStatus is marked as failed), then the resulting Future will be a failure too". It can be convenient for some use cases, but I think it's mostly misleading, because the Couchbase driver is a bit rough with failed operation in my opinion (a non existant key is no an error, it's a non existant key) and I tried to make the ReactiveCouchbase API in a way that you can deal with failures easily.

If you run the test with failfutures=true, then the resulting Future will be a failure and it will print Got some error : Not Found, but the test still passes because I use none.recover.

So yes, with failfutures=true the behavior that you described above is the expected behavior.

nielsboldt commented 10 years ago

The strange thing is that this behaviour just seems to have changed currently. I have just updated the snapshot of reactivecouchbase I was using for the version 0.2. I did a sbt cache clean because I'm trying to upgrade to 0.3.

So before this update I'm pretty sure that a non existing key would not mean an exception, but I'm not 100% sure. But the code I discovered my issue in, was running in production prior to the upgrade without any issues and after the update of snapshopt it failed big time :-(

Anyways, I guess the above means that failfutures=true is mostly useless and I will revert to false

Thanks Niels

mathieuancelin commented 10 years ago

I think that the feature is here almost since the beginning of the project

You can check here :

https://github.com/ReactiveCouchbase/ReactiveCouchbase-core/commits/master/driver/src/main/scala/org/reactivecouchbase/client/CouchbaseFutures.scala

The only change since 0.2 was :

https://github.com/ReactiveCouchbase/ReactiveCouchbase-core/commit/d2955a9da14716fc8edd9d65df02b10a84488098

nielsboldt commented 10 years ago

It was also quite puzzling. Thanks for your answers

/Niels