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.

Sign up for GitHub

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

Jump to bottom

ArC fixes for spec compatibility, round 4 #31248

Merged
merged 4 commits into from
Feb 23, 2023
Merged

ArC fixes for spec compatibility, round 4 #31248

merged 4 commits into from
Feb 23, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Feb 17, 2023

Related to #28558

  • ignore static methods annotated @Inject, similarly to static fields
  • fix dependency injection order
    • this is to pass the AtInject TCK
    • to review the BeanGenerator changes, I suggest to hide whitespace changes
  • implement CDI 4.0 startup/shutdown events
    • I believe it's best if we let current Quarkus events just extend the CDI 4.0 event classes, so that they are basically the same
  • improve bean archive detection
    • treat archives that contain Build Compatible Extensions just like we treat archives that contain Portable Extensions (which is what CDI requires)
    • archives that contain beans.xml with bean-discovery-mode="none" are no longer treated as bean archives

@Ladicek Ladicek requested review from mkouba and manovotn February 17, 2023 13:57
@Ladicek Ladicek mentioned this pull request Feb 17, 2023
Closed
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/core labels Feb 17, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor

mkouba commented Feb 21, 2023

The CI failures look related:

2023-02-20T19:28:06.6408580Z 2023-02-20 19:28:06,610 ERROR [io.sma.rea.mes.provider] (main) SRMSG00212: Unable to initialize mediator: io.quarkus.smallrye.reactivemessaging.signatures.ProcessorSignatureTest$BeanProducingAPublisherOfPayload#process: java.lang.ClassFormatError: Duplicate method name "getEmitter" with signature "()Lio.smallrye.reactive.messaging.annotations.Emitter;" in class file io/quarkus/smallrye/reactivemessaging/signatures/ProcessorSignatureTest_BeanProducingAPublisherOfPayload_ClientProxy

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 21, 2023

Yeah. I'll look.

manovotn
manovotn reviewed Feb 21, 2023
View reviewed changes
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 only have two minor comments.
Many thanks for diving into the AtInject TCKs :)

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 21, 2023 •
edited
Loading

The test failure shows exactly why I was originally hesitant to change Methods.MethodKey: it is often used to skip processing a method in case a method with the same name and signature has already been processed. This is not a definition of overriding (though it's close). I'll move back to having a separate detection of overrides. The reason why I need to detect overrides properly is of course the AtInject TCK, which verifies that overridden initializer methods are not injected twice, but not overridden methods with the same signature are.

@Ladicek
ArC: ignore static methods annotated @Inject, similarly to static fields
eccb0e7
@Ladicek
ArC: fix dependency injection order
b898208 
In case of class beans, dependency injection order is specified
by the AtInject specification as follows:

- injection points on superclasses are injected before injection points
  on subclasses;
- fields are injected before initializer methods.

Since we discover injection points bottom up, this requires reordering
after each class in the hierarchy is scanned for injection points.

The specification also prescribes that overridden initializer methods
are not injected at all. Since we discover injection points bottom up,
this requires remembering already seen methods and skipping methods
that are overridden by those that were previously seen.

This requires proper override detection. The `Methods.isOverridden()`
and `Methods.MethodKey` utils are not enough, because they actually
do _not_ detect overrides. They are used to skip processing a method
in case a method with the same signature has already been processed.
To do detect overridden methods properly, it is important to remember
that:

- private methods cannot be overridden,
- package-private methods can only be overridden in the same package.

For that reason, this commit introduces a dedicated override detection
utility.
@Ladicek
ArC: implement CDI 4.0 startup/shutdown events
b925085
@Ladicek
ArC: improve bean archive detection
a8af85b
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 23, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 23, 2023

Failing Jobs - Building a8af85b

Status Name Step Failures Logs Raw logs
✔️ Maven Tests - JDK 11
✖ Maven Tests - JDK 11 Windows Download Maven Repo ⚠️ Check → Logs Raw logs

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 23, 2023

I have no idea what the failure means -- the Windows job probably ran a lot later than the other jobs and the cached Maven repo .zip was no longer available? 🤔

@manovotn
Copy link
Contributor

I have no idea what the failure means -- the Windows job probably ran a lot later than the other jobs and the cached Maven repo .zip was no longer available? thinking

I've seen that before but don't know the cause either.
However, it shouldn't be related to the changes here, so I think it'd be safe to merge.

@mkouba mkouba merged commit 5f484d0 into quarkusio:main Feb 23, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 23, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 23, 2023
@Ladicek Ladicek deleted the arc-fixes branch February 23, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/core
Projects
None yet
Milestone
3.0.0.Alpha5
Development

Successfully merging this pull request may close these issues.
3 participants
@Ladicek @mkouba @manovotn