-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for non-blocking DataHandler #18
Conversation
As you know, OAuth2AsyncProvider of play2-oauth2-provider enables you to make asynchronous request that internally calls DataHandler like this:
I think RDBMS is commonly used datastore that DataHandler stores and finds credential data. As far as I know, there're few RDBMS drivers that support asynchrnous call at the moment and so I think making current interface completely asynchronus doesn't beneft for many users. Rather I prefer keeping its interface and implementation by users simple. However, we've received asynchronous support for DataHandler from others and I can understand what you say. Currently I'm thinking to provide both non-blocking and blocking interface so that user can choose preferable way. What do you think? To anyone who has an interest in this issue, I'd like to hear your opinion. |
I think providing both non-blocking and blocking interface for DataHandler is Good Idea~ We can use async RDBMS client driver for MySQL/PostgreSQL. and for modern(compared to RDBMS) NoSQL databases like Redis, MongoDB, ... and data service also can be processed through REST API(can be implemented fully async naturally by web frameworks) (In my case, I use Titan Graph Database through Rexster-Server REST API) :D thanks for scala-oauth2-provider project and upcoming async DataHandler :D |
Having only a future based one is far better than the current. You said
I would argue that is not true - it's not async. All it is doing is wrapping the result from the trait with a Future.successful() in the OAuth2AsyncProvider trait. In my opinion, this is extremely dangerous as it tricks developers into thinking they are actually using a non-blocking, future based call. It is dangerous because If I'm writing an application that is non-blocking and I tune my thread pool to be as such, if something blocks on that pool - then it can easily affect my entire application as I only tune my app to use the same number of threads as I have CPU's (the play default). In this case with the OAuth2AsyncProvider - what is really happening is that work does blocking things on the same exact thread as the request itself and then tricks the developer into thinking it was done on a future by mapping the result onto it. The best implementation would be to have both options a developer could implement - a future based one (like in this pull request) and the current (before my pull) - but without that implementation, mine is still better because if you want to block - it's no problem, you just wrap your implementation in the DataHanlder trait with Future.successful(...) - the same thing you are doing OAuth2AsyncProvider trait. Except this time - if an implementation is non-blocking, it operates exactly as it should - fully async. If an implementation is blocking - that is fine, as it puts that decision directly into the hands of the developer implementing the trait and he can decide if he really wants to block on something. Also - I would argue differently on the database side. In a microservices based architecture, I may end up using another service itself to handle the storage of tokens themselves or more likely - for validating a password grant type with an existing user micro-service that I already have - which I would call via my DataHandler trait in a non-blocking WS call. |
…er trait. Additional Comments: All tests passing, but probably needs a closer inspection particularly on some of the grants that throw exceptions as I don't believe those are currently being tested for each exception type.
Thanks for your feedbacks. @bwlim @rmmeans
I worry about to change the method signature of DataHandler for current our library users. On the other hand, I've tried just a little to support both non-blocking and blocking interface, I have felt it would be a little harder to maintain. Honestly, version of our library is still 0.x, and then I think we can change method signatures by the better change. Anyway, I will try to merge your PR into master branch. |
Sounds good - I will be spending significant time with this next week for my day job. If I come across anything more I'll send more PR's. I agree on your comment of the complexity of supporting both. I took a small look at that last night, and that does seem very complicated and would make maintaining this project much more difficult. Thanks for the good work - hopefully I'll be able to continue contributing as I have time. |
Merged into master branch. I did refactoring and added some test cases for future. |
@tsuyoshizawa Can you start publishing the snapshots? Noticing that the snapshot releases aren't being published: https://oss.sonatype.org/content/repositories/snapshots/com/nulab-inc/play2-oauth2-provider_2.11/ it would be easier for us to validate new releases with this. Thanks again! |
Hi, @rmmeans LGTM for my application by using the snapshots. Thanks for your cooperation. |
I forgot about sbt "+ publishLocal" too... that worked. Yes I will provide feedback as I scope out our first implementation against the snapshot. It may be another week or two before I have good feedback however. Thanks! |
…to master * commit '4363708443cf6dfe3f926a62011e3ff558d6a179': chore() Merge upstream. Parent was redefined as trait so we had some class compatability issues. bugfix() Forgot to commit these chore() Bump dependency versions testing for scala 2.10.x and 2.11.x on travis enhance validating to parse Authentication header add description for DataHandler with Future improve error messages add OAuth2Provider customize supporting grant types fix description Refactoring and add test case nulab#18 update a version for Playframework 2.2 When DataHandler.findUser() returns None, (incorrect user/password), Returns InvalidGrant with a more decriptive error message update README for how to create your trait First pass on moving the library to support a non-blocking data-handler trait. version 0.8.0
This is a quite a bit of a refactoring. I'm guessing you'll want to review this with some current implementations of the service as I currently don't have any that I can vet it with. We are moving there, but I needed it to support the data-handler to be actually non-blocking based upon Futures. Otherwise, the Async helper for the Play Framework really doesn't do anything in an asynchronous style. Assuming that storing tokens, finding the user, authenticating a password was done in some sort of database, then this really needed to be able to be accomplished in a non-blocking way.
It's important to note that this will assume a hard cutover for current users as the trait's signature has changed. Assuming some documentation could be added to say if you want to continue to use it as-is in a blocking manner just wrap your implementation in a Future.successful{ ... }