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

In the FullStackTest, the EntityManagerProvider incorrectly relies on disposal method #376

Open
manovotn opened this issue Jul 14, 2022 · 5 comments

Comments

@manovotn
Copy link
Contributor

This was originally brought up in #375

The provider in question is [this class](https://github.com/smallrye/smallrye-context-propagation/blob/main/tests/src/main/java/io/smallrye/context/test/EntityManagerProvider.java#L29-L32.
This logic won't work if there is CP involved and Weld is used. It is documented in this part of Weld docs but in short, the way we propagate in CDIContextProvider is that we only ever deactivate contexts, but never invalidate them so all pre destroy and disposer methods are skipped. This is because otherwise we would invoke them multiple times, potentially leading too unexpected states.

For what we test, this doesn't matter but it is showing a state that doesn't work and in real app, the EM would never close.

Therefore, we should:

  • Move the em.close() logic elsewhere. Not sure where, maybe @BeforeDestroyed(RequestScoped.class) and we should check if the bean exists before doing that.
  • Also assert that pre-destroy and dispose methods aren't invoked with CP in place
@DavideD
Copy link

DavideD commented Jul 14, 2022

I would add that not closing the entitymanager can cause issues that are hard to debug.

For example, a similar issue happens in Quarkus when using @ActivateRequestContext with a reactive method. Because the dispose method is never called, a TimeoutException happens after 3 queries (it changes depending on the PC).

If possible, it would be nice to warn users if we already know that the dispose method won't get called.

@manovotn
Copy link
Contributor Author

The Quarkus issue is, in essence, different. There, the disposal should be called but it was handled incorrectly from what I can tell. In Weld, this is currently a limitation and a hard one to bypass at that (mainly because of support for session/conversation).

We could maybe add some INFO one-time logging into the CDI context provider in this project to let people know?
CC @FroMage

@DavideD
Copy link

DavideD commented Jul 14, 2022

Yes, I mentioned the issue to highlight the impact of not calling the dispose. And the importance of letting users know about these situations in advance.

@FroMage
Copy link
Contributor

FroMage commented Jul 15, 2022

We could maybe add some INFO one-time logging into the CDI context provider in this project to let people know?

How do you detect that?

@manovotn
Copy link
Contributor Author

We could maybe add some INFO one-time logging into the CDI context provider in this project to let people know?

How do you detect that?

What I meant is to simply LOG an info that CP in combination with Weld is used and that users shouldn't rely on predestroy/dispose methods. This could be done in[ CDIContextProvider](https://github.com/smallrye/smallrye-context-propagation/blob/main/cdi/src/main/java/io/smallrye/context/cdi/context/propagation/CdiContextProvider.java) in its constructor or once it is first used. Something along those lines.

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

No branches or pull requests

3 participants