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

Only call entityManagerContext.pop if the EntityManager is set #7123

Merged
merged 1 commit into from Apr 17, 2017

Conversation

Projects
None yet
4 participants
@fjtorres
Copy link
Contributor

commented Mar 23, 2017

Fixes #6041 and #7037

Francisco Javier Torres
@@ -153,8 +153,8 @@ public EntityManager em() {
}
throw t;
} finally {
entityManagerContext.pop(true);

This comment has been minimized.

Copy link
@benmccann

benmccann Mar 24, 2017

Contributor

Maybe a separate if check makes sense here instead of checking the value of a different variable and assuming they're always the same (which even if true now has more potential to break later):

if (entityManagerContext != null) {
  entityManagerContext.pop(true);
}

This comment has been minimized.

Copy link
@fjtorres

fjtorres Mar 27, 2017

Author Contributor

The if must check entityManager because the problem of the associated issue occurs when the entityManager is not pushed in the entityManagerContext. So the call to pop method only should occurs when the entityManager is not null.

You can check issues #6041 and #7037 to get more information.

@gmethvin
Copy link
Member

left a comment

The description should explain what the PR actually does. e.g. "Only call entityManagerContext.pop if the EntityManagerContext is set".

@fjtorres fjtorres changed the title Resolve issue #6041 and #7037 Only call entityManagerContext.pop if the EntityManager is set Mar 27, 2017

@marcospereira
Copy link
Member

left a comment

LGTM.

@gmethvin gmethvin merged commit 3db5f91 into playframework:master Apr 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

gmethvin added a commit that referenced this pull request Apr 17, 2017

@marcospereira marcospereira added this to the 2.5.15 milestone Apr 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.