Skip to content
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

@Transactional and TransactionalAction conception problem - does not work with Secure Social plugin #11

Open
rlamarche opened this issue Apr 10, 2015 · 3 comments

Comments

@rlamarche
Copy link

Hi,

I've found a critical bug in the play java ebean plugin.
In my opinion, there is a confusion in the usage of promise and action wrappers (@With ...).

Look at this portion of code from the class play.db.ebean.TransactionalAction: (https://github.com/playframework/play-ebean/blob/268c771426398353af39c0c45fb458122cc57a6b/play-ebean/src/main/java/play/db/ebean/TransactionalAction.java)

/**
 * Wraps an action in an Ebean transaction.
 */
public class TransactionalAction extends Action<Transactional> {

    public F.Promise<Result> call(final Context ctx) throws Throwable {
        return Ebean.execute(new TxCallable<F.Promise<Result>>() {
            public F.Promise<Result> call() {
                try {
                    return delegate.call(ctx);
                } catch (RuntimeException e) {
                    throw e;
                } catch (Throwable t) {
                    throw new RuntimeException(t);
                }
            }
        });
    }

}

The EBean transaction starts before entering the public F.Promise<Result> call() method, and is commited or rollbacked depending if there is an exception thrown at the end of call().

In this callable, the result of delegate.call(ctx) is returned. This result is of kind F.Promise<Result>.
A promise is a "promise of response", but nothing tell us that the underlying action has already been executed after the delegate call. Moreover, nothing tell us that it will be executed in the same thread.

In the common case where no play plugins are used, this portion of code works: but I insist on the point that this case is a special case, because I don't know why but when the F.Promise is returned, the underlying action has already been executed in the current thread. I think that this is a misconception but that's my opinion.
Moreover nothing forbid in the current API to execute the delegate action in another thread and later (I mean after the transaction commit).

For example, if you use the popular and almost vital plugin Secure Social, this code does not work anymore. There is no exception thrown, but the subsequent action is executed outside the transaction, and this is a very bad idea.

Indeed, in this case, the delegate returns a F.Promise where the action has not yet been executed. And again, I insist on the point that this is not a Secure Social bug in my opinion because the promise API is made to allow that!

For information, I'm using Secure Social 3.0-M3 and Play Framework 2.3.8.

Currently, I'm trying to understand how to really "wrap" the action execution using the F.Promise API, but any help would be really appreciated.

@rlamarche
Copy link
Author

Note that EBean is using a ThreadLocal internally to store the current transaction, like a lot of ORMs.

@lfleuriot
Copy link

👍

@jroper
Copy link
Member

jroper commented May 5, 2015

There are a number of issues here. Firstly, I think it is possible to provide a custom thread local scope for ebean, such that the transaction is bound to Play's http context, and then have the transaction committed/rolled back in the callbacks attached to the returned future. However, doing this is prone to deadlocks, due to the fact that the action could block on the connection pool, while the request with a connection could be stuck in the thread pool queue waiting for the thread blocking on the connection pool to finish executing.

The best approach for now is to explicitly use Ebean.execute yourself. I think a proper solution to this would be to create an AsyncTransactional annotation, with clear docs on it about the potential deadlocks that could occur.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants