Revert change client proxy class package #23185
Conversation
I don't think that we should revert the fix for #22815 because #23167 only affects the dev mode while the original issue breaks an app in prod mode. I believe that quarkus.class-loading.reloadable-artifacts can be used as a workaround. |
|
@mkouba well except the case of @Postremus looks relatively common and is a regression? Whereas the other case requires the open telemetry extension. I certainly don't want us to introduce a regression to fix something that wasn't working before. |
Well, it requires a normal-scoped producer bean placed in the application and this producer must return a type from an external library.
For me the regression has a lower priority because 1) it's dev mode only, 2) there is a workaround. |
|
Well, dev mode is the normal mode for developing applications. So it's not less important than production mode. And while I think a workaround for @Postremus is acceptable, I wouldn't want us to have a ton of people complaining about this and having to implement the workaround. I would have agreed with you if I had to choose between two regressions but here we have to choose between introducing a regression and fixing something that wasn't working. So my personal opinion is that we should revert for |
|
BTW, I'm not saying that this should be reverted forever. I just think we need more time to think about what to do and probably involve @stuartwdouglas and @geoand . |
My point is that we can't gurantee the same level of stability for the dev mode. Of course, the dev mode is very important but there were, there are and there will be issues originating from the difference in classloading. That said, if I should choose between a regression in the dev mode and a fix for the prod mode I'd take the latter.
+1 |
|
I think it depends on what we would like to see as a proper fix, base/runtime split and how we achieve it. If we are not yet sure the current fix is in line with that, it makes sense to discuss it more. After looking into a few classloading issues recently that are due to the base/runtime split, I think we should review the way we are doing it. I think it's about time. It looks like it was inevitable to run into these issues with maturity of the project. |
BTW this is not the case. It's just that the other issue is demonstrated by the telemetry extension, i.e. it's a more general problem. |
Let's agree to disagree :). We are advertising dev mode as the primary way to develop applications so breaking it is a problem. Even if there is a workaround. I prefer keeping something that was not working until now. I will pursue with the revert as soon as CI is green and I'll let you, @aloubyansky, @stuartwdouglas and @geoand discuss what we should do for 2.7.1.Final. |
🤷 |
|
So I was looking at how Just to be clear - the original fix in #22828 changed the package where a client proxy is generated for a CDI producer. Previously, it was the package of the declaring bean class. After the fix, it's the package of the return type. This setup prevents from |
Seems fair enough to me... |
|
The tests added in this PR would fail now in main, right? My question would be whether the generated proxies should either always be among the reloadable deps or not. |
|
Ok, they are in the runtime CL while the classes the proxies were generated for are in the base CL. I need to look into the tests. |
Yes, the reverted tests should fail in main.
Hm, I think that it would not help to place all generated proxies in the runtime CL because we won't be able to access package-private members anyway (because the classes need to be loaded from the same ClassLoader). Also think about split-packages... |
|
Right. So if we re-applied your fix, for a use-case like in the #23167 reproducer, making the client proxy an "application class" would work. But if it's modified to mimic the test you added, i.e. making the |
@mkouba i'm opening this one in the meantime. Unless we find a proper fix for 2.7.0.Final by tonight, we should merge this one and look for a proper fix for 2.7.1.
Fixes #23167