smallrye / smallrye-context-propagation

SmallRye implementation of MicroProfile Context Propagation
Apache License 2.0
30 stars 24 forks source link

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

Open manovotn opened 2 years ago

manovotn commented 2 years ago

This was originally brought up in https://github.com/smallrye/smallrye-context-propagation/issues/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:

DavideD commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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` in its constructor or once it is first used. Something along those lines.