Add support for Play 2.6

[mkurz]

This pull request adds support for Play 2.6.

To make it easy to follow the changes I split them in smaller commits. @mumoshu I think it's best if you review commit after commit to follow my thoughts.

As you will see in the commits I changed the play.modules.cache.* config keys to play.cache.* because that is what Play is using for it's own cache plugins as well (I think using play.modules.cache has always been wrong).

For the new version (only) I upgraded spymemcached to the latest version which let us use the (new) .addListener(...) method. This - in combination with the fact that since Play 2.6 the cache api is asynchronous per default - allows us to completely get rid of any blocking get method and therefore we are finally completely non-blocking! :smile: :tada:

Bonus: I also (re-)implemented #26 (Hash keys before each operation) and #30 (Exceptions from get)

(FYI: For some of the changes I had a look at the EhCacheApi.scala)

[mkurz]

@mumoshu One of the test Build Jobs fails - however this has nothing to do with my changes. Maybe you can restart the failing Build Job?

[wwbakker]

@mkurz Thank you very much for your contribution.

[mkurz]

@mumoshu do you think you find time the next days to review/merge this pull request?

[wwbakker]

By the way, this friday I've worked all day on trying to get it to work for our application, which uses AWS' elasticache but I haven't gotten it to work yet.

What happens is that a timeout occurs from the setter in SyncCacheApi. Changing the timeout from 5 seconds to 30 seconds doesn't change it: it still times out, just later. The wierd thing is that I've started debugging it, and I see that some of the sets work, but others time out. My current theory is that it could have something to do with execution contexts. What at least doesn't seem completely correct to me is that for the getter, the future is turned into a scala future, but for all the other commands the java future returned by the memcached client is ignored, and 'Done' is returned.

I'll have to look into it further next week to debug what is the problem.

Currently I've copied over the code, instead of using your fork, because we're on play 2.6.2, so it's possible (though not for sure) that the pull request is completely working. Another thing that's making it more difficult is that amazon supplies it's own net.spy.memcache-java-client which is a fork of the original one. Our play 2.5 version of the application (which still works) uses a fork of that version of net.spy spymemcached that work in conjunction with play. But that is still based on the old version of net.spy.

@mkurz Maybe you could update your pull request to use play 2.6.2 since that's the current latest version? Most likely it's a question of doing a search and replace.

[mkurz]

@wwbakker

What happens is that a timeout occurs from the setter in SyncCacheApi. Changing the timeout from 5 seconds to 30 seconds doesn't change it: it still times out, just later.

Which config key did you change? Can you post the exception?

What happens is that a timeout occurs from the setter in SyncCacheApi

Does the timeout only occur for the setter? What about the other methods (getter, remove, etc.)

About the futures, I will have a look.

[wwbakker]

@mkurz

Which config key did you change?

There is no config to change. What I did was override the DefaultSyncCacheApi (java version) and change the value of the protected timeout field in the constructor after the super() call. After that I changed the MemcachedModule so that the extended version was bound, instead of the default one.

However this was not the problem: In our 2.5 branch (based on the 2.4 version of this plugin), which still works, it doesn't even take close to 5 seconds.

Can you post the exception?

I can't right now, but it was a very generic java/scala concurrent timeout iirc. The issue didn't seem to be the timeout however. It seemed to be that the setter was called, but the future did not return.

Does the timeout only occur for the setter? What about the other methods (getter, remove, etc.)

I'll have to look into that. The application didn't get that far.

Sorry, I don't think you can find a solution for me using the information I have for you here. I'll have to look into it/debug it myself. Thank you for trying though.

@mkurz Are you using this plugin yourself for something? What kind of memcached are you using? self-hosted? aws? some other cloud service? Does it work for you?

[mkurz]

@wwbakker Please update your code with the commit I just added and try again. I now use the callback approach for all other methods as well (set, remove, etc.). For me it seems something is blocking in your code. Did you just try in dev mode when only one thread is used? Did you already try running in prod mode e.g. by running sbt stage; ./target/universal/stage/bin/<your-application-start-script>?

[wwbakker]

@mkurz ok, so my initial hunch was correct: The problem is the execution context. Before you made this pull request I already begun upgrading this plugin myself. My version however still used the older net.spy version, but was blocking, like this:

  override def set(key: String, value: Any, expiration: Duration): Future[Done] =
    if (!key.isEmpty) {
      val exp = if (expiration.isFinite()) expiration.toSeconds.toInt else 0
      Future {
        blocking {
          client.set(namespace + key, exp, value, tc).get()
        }
      }.map(_ => Done)
    } else Future.successful(Done)

That didn't work either, however I just changed the ExecutionContext from the one provider by guice/play with ExecutionContext.global and now it works perfectly.

Obviously this is a work around and will not be ok in the final product, but now I should be able to upgrade things 1 by 1. I'll try with your new commit

[mkurz]

@wwbakker Did you just run in dev mode?

[mkurz]

@wwbakker I think you could also set the play.cache.dispatcher config to make it work. See https://github.com/playframework/playframework/blob/2.6.x/documentation/manual/working/javaGuide/main/cache/JavaCache.md#setting-the-execution-context I just introduced this setting in Play a couple of days ago: https://github.com/playframework/playframework/pull/7649 / https://github.com/playframework/playframework/pull/7672 (And it's available in the play-memcached plugin via this pr here as well :smile: )

But I think just upgrading to my commit should make it work for you anyway.

[wwbakker]

@mkurz Yes, now the setter doesn't use execution context anymore, it works, even with the newest version of net.spy.

Thanks for your help!

the getOrElseUpdate still uses it though, so I'm not sure if that works (my application doesn't invoke that method).

[mkurz]

I don't think the getOrElseUpdate will make a problem because it doesn't use the memcached client itself. However could you maybe try the method by invoking it with some test data? Just to make sure it doesn't make any problems on aws.

[mkurz]

However for some reason I still think this problem just occured in dev mode when only one thread is available...

[mkurz]

I also upgraded to Play 2.6.2 and latest Scala 2.12.3 now and everything works without a problem.

[wwbakker]

@mkurz

However for some reason I still think this problem just occured in dev mode when only one thread is available...

No it's not just dev mode. Elasticache is not accessible locally (only from instances within aws) so I had to set up a ssh tunnel to debug it locally, which also took some time friday. I actually found out the problem on our development/test servers (which don't run in dev mode).

Also, I'm pretty sure devmode isn't single threaded.

However could you maybe try the method by invoking it with some test data? Just to make sure it doesn't make any problems on aws.

  def test = Action { request =>

    cache.set("testkey", "value", 20.seconds)
    val v1 = cache.getOrElseUpdate("testkey", 20.seconds)("orElseValue")
    val v2 = cache.getOrElseUpdate("testkey2", 20.seconds)("orElseValue2")
    Ok(v1 + v2)
  }

result valueorElseValue2

Seems good to me. 👍

I also upgraded to Play 2.6.2 and latest Scala 2.12.3 now and everything works without a problem.

Great.

[mkurz]

Perfect! Thanks for testing getOrElseUpdate :+1: Using the callbacks here is the best approach anyway.

[mkurz]

@mumoshu I am pretty sure this pull request is ready to merge. Could you please have a look and release a new version as well? Thanks!

[mumoshu]

@mkurz LGTM. Thank you very much for your impressive works from play to this plugin 👍 I'll try to release the plugin asap.

[mkurz]

Great! Thank you!

[mumoshu]

Just released artifacts to Sonatype. They will probably be synced to the central shortly 😉