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

ArC: fix the situation when a framework bean uses an application decorator #43245

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Sep 12, 2024

ArC-generated classes for framework beans are by default not application classes. This causes problems in hierarchical classloader environments (dev/test mode) when there is an application decorator that applies to the framework bean.

This commit fixes that issue by turning the ArC-generated classes for framework beans into application classes whenever an application decorator applies. This makes package access impossible, which is an unfortunate downside.

The problem doesn't exist in a flat classloading environment, such as prod mode.

…rator

ArC-generated classes for framework beans are by default _not_ application
classes. This causes problems in hierarchical classloader environments
(dev/test mode) when there is an application decorator that applies to
the framework bean.

This commit fixes that issue by turning the ArC-generated classes for framework
beans into application classes whenever an application decorator applies. This
makes package access impossible, which is an unfortunate downside.

The problem doesn't exist in a flat classloading environment, such as prod mode.
@Ladicek Ladicek added the area/arc Issue related to ARC (dependency injection) label Sep 12, 2024
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 12, 2024

I couldn't figure out a better solution. I tried turning all loadClass() calls in ArC into loadClassFromTCCL(), but that didn't help, so this is the only solution I can think of.

Copy link

quarkus-bot bot commented Sep 12, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 077f1a9.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

🎊 PR Preview 62039a3 has been successfully built and deployed to https://quarkus-pr-main-43245-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

Copy link

quarkus-bot bot commented Sep 12, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 077f1a9.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/virtual-threads/grpc-virtual-threads

io.quarkus.grpc.example.streaming.VertxVirtualThreadTest.testUnary - History

  • INTERNAL: Half-closed without a request - io.grpc.StatusRuntimeException
io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request
	at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:268)
	at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:249)
	at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:167)
	at io.grpc.testing.integration.TestServiceGrpc$TestServiceBlockingStub.unaryCall(TestServiceGrpc.java:220)
	at io.quarkus.grpc.example.streaming.VirtualThreadTestBase.testUnary(VirtualThreadTestBase.java:33)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:971)

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix seems to be reasonable.

I wonder if we should mention this in the CDI ref guide 🤔.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is the best we can do to make the behavior consistent.

This commit fixes that issue by turning the ArC-generated classes for framework beans into application classes whenever an application decorator applies. This makes package access impossible, which is an unfortunate downside.

I wonder if we should mention this in the CDI ref guide

We could have a configuration knob for the previous behavior which would enable the pack private access and also serve as a documentation. Although I am not sure it's worth it.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 13, 2024

To be honest, I'd rather do a bytecode transformation that turns package-private injection points (and other things, probably?) into protected fields/methods :-) That should fix the package access issue.

Documenting the package access restriction in the CDI reference guide just seems like accepting defeat.

@manovotn
Copy link
Contributor

To be honest, I'd rather do a bytecode transformation that turns package-private injection points (and other things, probably?) into protected fields/methods :-) That should fix the package access issue.

I don't think we want to do this everywhere and detecting just these cases might be challenging.
It is definitely an option if the problem surfaces again though.

Documenting the package access restriction in the CDI reference guide just seems like accepting defeat.

Yea, plus the CDI ref. guide is too broad for this peculiar case.

@mkouba
Copy link
Contributor

mkouba commented Sep 13, 2024

We could have a configuration knob for the previous behavior which would enable the pack private access and also serve as a documentation. Although I am not sure it's worth it.

Configuration knob is probably an overkill.

To be honest, I'd rather do a bytecode transformation that turns package-private injection points (and other things, probably?) into protected fields/methods :-) That should fix the package access issue.

Hmm, keep this idea for later..

Documenting the package access restriction in the CDI reference guide just seems like accepting defeat.

It's a small defeat. And it's ok to admit such things. It's a downside of the dev/test mode after all 🤷.

@mkouba
Copy link
Contributor

mkouba commented Sep 13, 2024

Documenting the package access restriction in the CDI reference guide just seems like accepting defeat.

Yea, plus the CDI ref. guide is too broad for this peculiar case.

A single line in the https://quarkus.io/guides/cdi-reference#supported_features_and_limitations after "Decoration of built-in beans, such as Event, is not supported" would be enough.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 13, 2024

I'm afraid it's not gonna be just a "single line", or it's gonna be a long single line. How about this?

If a decorator provided by the application applies to beans provided by external libraries, package-private injection points (and other CDI constructs) are not going to work in those beans in dev/test mode.

Sounds fairly crazy to me, but if you agree, I'll add that.

@gsmet
Copy link
Member

gsmet commented Sep 13, 2024

Please make it clear what you should use and actually work in the sentence. It’s not always clear to our users.

@mkouba
Copy link
Contributor

mkouba commented Sep 13, 2024

Please make it clear what you should use and actually work in the sentence. It’s not always clear to our users.

@gsmet Well, the problem is that users can't do much in this particular case because those problematic injection points are defined in external libraries/extension runtime artifacts...

@manovotn
Copy link
Contributor

Please make it clear what you should use and actually work in the sentence. It’s not always clear to our users.

@gsmet Well, the problem is that users can't do much in this particular case because those problematic injection points are defined in external libraries/extension runtime artifacts...

Yes, in this particular case the user might not even know their decorator applies to a bean provided by some other fwk.
Which is also why I think the documentation doesn't make much of a difference.

@mkouba
Copy link
Contributor

mkouba commented Sep 13, 2024

I'm afraid it's not gonna be just a "single line", or it's gonna be a long single line. How about this?

If a decorator provided by the application applies to beans provided by external libraries, package-private injection points (and other CDI constructs) are not going to work in those beans in dev/test mode.

Sounds fairly crazy to me, but if you agree, I'll add that.

@Ladicek You're right, sounds crazy and most readers won't be able to decrypt this message. So we should probably keep it as a secret for now 🤷.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 16, 2024

Any objection to merging this?

@rsvoboda
Copy link
Member

@gsmet gsmet removed this from the 3.16.0.CR1 milestone Nov 13, 2024
@gsmet gsmet added this to the 3.15.2 milestone Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants