Alter the library methods to propagate an implicit ExecutionContext

[joaoraf]

Details on issue #341.

[joaoraf]

Forgot to add that it pass all tests. I had to change the tests due to the extra ExecutionContext parameters.

[akkie]

This comment can be removed

[akkie]

You are my hero! Really! This was the next task on my to do list and now you save me a lot of time.

There are two things I've noticed. The first are the comments with the old implicit execution context. They are widespread over the complete project. These should be removed. The other is a question regarding the equality matcher you've used in the tests:

env.identityService.retrieve(===(identity.loginInfo))(any) returns Future.successful(None)

Why is it necessary now? Could you also explain what the smart mock does? I'm not aware of such a functionality.

@rfranco Please could you also review the pull request?

[joaoraf]

Sorry about the comments. I thought I had removed then before commiting.
I had lots of trouble with the matchers. I actually had to learn about Mockito, which was a good thing.
Using your example, the original expression was:
env.identityService.retrieve(identity.loginInfo) returns Future.successful(None)
Since there wasn't any matchers inside the expression "env.identityService.retrieve(identity.loginInfo)"
However, now we have a implicit ExecutionContext parameters and we need to add a wildcard (any)
matcher to the expression, otherwise Mockito freaks out:
env.identityService.retrieve(identity.loginInfo)(any) returns Future.successful(None)
Now mockito complains that there should be two matchers instead of just one. The rule is, if a matcher is
used, then all parameters must be matchers also. Thus we need:
env.identityService.retrieve(===(identity.loginInfo))(any) returns Future.successful(None)
I hope that explains it.
I will try to amend the commit (I am also been (willingly) forced to learn to use GitHub and git)
I am really happy to be useful. Your library is very useful and the code is extremely elegant.

[joaoraf]

About the smart mock: I had some problems with null values being returned from unexpected unstubbed methods after I started adding the ExecutionContext parameters.
I eventually found out that I had to pass the implicit parameters explicitly in those methods, otherwise Mockito would not know how to stub then and would just return null.
To find these out I used the smart option, which makes the mock return a SmartNull instead of a null value, resulting in a error message which explains which method was unstubbed (see https://code.google.com/p/specs/wiki/UsingMockito#Smart_mocks).

[akkie]

Ahhh, now I see. Thanks for the explanation.

[rfranco]

@akkie I'm traveling now and i'll be back Tuesday then i can do better review so i did a quick review and looks great anyway maybe it could be done as we did with MessagesApi, add the ExecutionContext on the Environment that dont need to spray the ExecutionContext for everywhere.

[akkie]

@rfranco But then we must spread the environment around.
@joaoraf Please could you merge the changes from #340 into this pull request?

[joaoraf]

In an hour.

[joaoraf]

@akkie @rfranco The build failed in a test. I don't see what the problem is. In my local code the tests run ok.

On CookieSecretSpec:

[info] The publish method of the provider should
[error] x add the secret to the cookie
[error] 'Some(Cookie(OAuth1TokenSecret,2-YEQIlweDazjnTbHwd3q1M3JhdVAxUcb4EC8IjXQSiZ7XMbSj/I/Bu4bFLFjAAtqORRvgycnEg0AjCQlJ2wglhw==,Some(299),/,None,true,true))' is Some but 'Some(299)' is Some but '299' is not equal to '300' (CookieSecretSpec.scala:125)

[joaoraf]

@akkie @rfranco seems to be ready now

[akkie]

@joaoraf The test error has to do with playframework/playframework#3120. Maybe we should change the test. Anyway, this shouldn't be a part of this pull request.

Please could you squash the commits into a single commit. Otherwise, for me it looks ready to merge.

[akkie]

@joaoraf I think the Authorization.isAuthorized method needs also an execution context.

[joaoraf]

@akkie everything tested and squashed.

[akkie]

@joaoraf The import comments are still available.

[joaoraf]

Damned squashing!

[rfranco]

I reviewed the code and i'm not sure about add ExecutionContext in everyplace.

One example is if create a IDGenerator that is force to receive the ExecutionContext on the method generate and can not accept in the constructor. So who call the method need to choice which ExecutionContext should be used otherwise it's not possible to inject the ExecutionContext on the implementation.

class FooIDGenerator() extends IDGenerator {
  def generate(implicit ec: ExecutionContext): Future[String]  = Future("foo")
}

VS

class FooIDGenerator(implicit ec: ExecutionContext) extends IDGenerator {
  def generate(): Future[String]  = Future("foo")
}

My suggestion is to review all place where need ExecutionContext and always prefers the constructor style.

[joaoraf]

@rfranco
I believe we may need to consider this carefully. In some cases, it makes perfect sense to fix an ExecutionContext, such as on the end of the call chain, where we usually block on I/O, such as when accessing a database, a remote service, etc.
Slick, for example, uses its own dispatcher inside its DBIO Monad.
A remote access for a specific Auth Service may require a pool of threads of its own or, its use may be so rare that it is not worth the extra cost of setting up another ExecutionContext.
Between the layer above Silhouette and the bottom layer (whatever it is), I believe it is best to use the calling point execution context, since the code in the middle is just CPU bound (and not much intensive at that).

What do you think?

Whatever you decide, I will try to make it.

Another path is to make it configurable somehow, allowing the user to choose (or inject) ExecutionContexts where such are needed.

[akkie]

I see it similar to @joaoraf. An execution context should be defined context bound and not at initialization time(for classes that gets typically injected). For helper or internal classes that gets typically constructed inside the code, a constructor injected execution context makes sense.

I've searched through Play, Slick, ReactiveMongo and Akka, and all the projects prefers the execution context on the method instead of the constructor.

Anyway, I'm not sure if the execution context must be defined as argument on every method which returns a Future. Maybe we should all three look over the code and create a list with methods that should be reconsidered.

[joaoraf]

Remember that you need a ExecutionContext whenever you manipulate a future using map, flatMap, etc. To create a Future you also need an ExecutionContext. What I believe should be avoided is the need to use the "scala.concurrent.ExecutionContext.global" execution context just because there is none when you need, for example, to map (or flatMap) the value of a future before returning to the caller.

[rfranco]

@akkie @joaoraf agree with both usually i prefer to inject ExecutionContext on Service, Repository and DAO because these could have syncs calls or necessary different context as UserService (JDBC) and Cache (Redis).

So we really should review all the methods and see which need each approach by method or constructor.

[akkie]

@joaoraf I meant methods that gets implemented by the user itself. Because the user can then define which execution context should be used. For methods that are already implemented by us the user should be able to pass the execution context.

Here are some examples for methods that gets implemented by the user:
SecuredSettings.onNotAuthenticated
SecuredSettings.onNotAuthorized
Silhouette.onNotAuthenticated
Silhouette.onNotAuthorized

DAOs, Services and repositories gets also implemented by the user. And I'm really unsure if it's the best approach to let the user handle the different methods over different execution contexts. So I'm with @rfranco here, to let the user inject the execution context if needed.

Anyway, I'm really unsure about the right approach. If I see it right then the current implementation provided by @joaoraf gives a user the possibility to define the execution context on controller level and then let the Silhouette depended code run in this execution context. A user is also able to define an other execution context in sub components of Silhouette.

If we implement only constructor injection of the execution context, then the user can only define the execution context for all controller instances and the dependent Silhouette classes once. As like with the other approach a user is able to define another execution context in sub components of Silhouette.

So the main question should be: Should the execution context be defined once or should it be defined per logically-related section of code?

From the doc:

However, application developers should carefully consider where they want to set policy; ideally, one place per application (or per logically-related section of code) will make a decision about which ExecutionContext to use. That is, you might want to avoid hardcoding scala.concurrent.ExecutionContext.Implicits.global all over the place in your code. One approach is to add (implicit ec: ExecutionContext) to methods which need an ExecutionContext. Then import a specific context in one place for the entire application or module, passing it implicitly to individual methods.

[akkie]

Can we find any consensus here?

[joaoraf]

Since Silhouette is intrinsically coupled to Play, we could use Play's ExecutionContext, which we can obtain through play.current or through the request (I believe) all way up to the calls to Services and Repositories (avoiding passing it through the call chain). The Services and Repositories then decide what they will use. They can use the Play's execution context also. We should just warn the user to avoid using the global execution context, using the Play one instead if they don't require a separate context for the Repository/Service.

As I said, whatever you decide I help implement. I am getting used to trace all these calls on the code ;-)

[akkie]

I've created a topic is the Scala-Users mailing list with the title "Best strategy to pass the ExecutionContext for a library". Maybe this can help us to find the best solution.

[joaoraf]

A very very nice discussion by the way. It seems this affects many people.

[akkie]

@joaoraf Would it be OK for you if I create a new pull request. I find it hard to make decisions based on this pull request. I think I must rummage through the code to find the places which needs the ExecutionContext.

[joaoraf]

No problem. Do you want some help? Have you decided the strategy?

[akkie]

Fixed with #360