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

Revisit the non-standard behavior of ArcContainer#instanceSupplier() #28429

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Revisit the non-standard behavior of ArcContainer#instanceSupplier() #28429

merged 3 commits into from
Oct 11, 2022

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Oct 6, 2022

Fixes #28115

This would be a breaking change in ArcContainer#instanceSupplier() because there can be other extensions (outside of this repo) that use this method unknowingly via BeanContainer#instanceFactory and those might rely on the non-CDI behavior that the underlying resolution performed.

Theoretically, we could instead deprecate instanceSupplier() method (as well as the method on BeanContainer) and keep their current behavior while introducing new ones with similar names that would behave per spec. That'd give extension creators time to adapt. WDYT @mkouba?

As for our own extensions, I am stuck with RE reactive and its handling of @Provider. The second commit shows the issue - the altered test would lead to ambig. exception (expected due to changes in BeanContainer#instanceFactory) but I am adding @Typed to all providers. Despite the fact that RE will register such provider, but will not use it during test execution. I might need someone with RE internals knowledge to take a look and tell me what I am missing...

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/rest area/undertow labels Oct 6, 2022
@manovotn
Copy link
Contributor Author

manovotn commented Oct 6, 2022

Added commit from @geoand that solves the issue I encountered.
We'll still need to make the RR transformation more refined

@manovotn manovotn mentioned this pull request Oct 6, 2022
Closed
@manovotn
Copy link
Contributor Author

manovotn commented Oct 7, 2022

I've updated the RR processor to transform annotations using RR build items as per @geoand suggestion.

I also noticed that I can still get intermittent failures of the ChunkedResponseTest - running the test repeatedly sometimes yields a failure locally which means that fc76c76 didn't fix it completely.

@geoand
Copy link
Contributor

geoand commented Oct 7, 2022

Thanks for the update. I'll have a look on Monday.

@geoand
Copy link
Contributor

geoand commented Oct 10, 2022

I pushed two commits:

  • One that reverts my previous fix (this should be squashed with the commit that introduced the change - I didn't want to do this myself as it's not branch so having to rebase would come as a surprise to you)
  • One that introduces a proper fix (or at least what seems to be a proper fix as I ran the tests multiple times and they always passed)

@manovotn
Copy link
Contributor Author

manovotn commented Oct 10, 2022 •
edited
Loading

@geoand silly question but did you rebuild all the parts? I am still seeing the failure and it is not even intermittent now - I am getting it on every run.

I think you must have run it without the revert commit you made (c441cc6) since with that sorting in place, I am not seeing any failures?

@geoand
Copy link
Contributor

geoand commented Oct 10, 2022

You are right, seems I must have missed something.

@geoand
Copy link
Contributor

geoand commented Oct 10, 2022

The reason we don't want my initial commit is that it breaks the TCK, so I need to figure out a different approach

@geoand
Copy link
Contributor

geoand commented Oct 10, 2022

Should be fixed now. My comment about squashing the two sorting commits I mentioned above still applies.

@manovotn
Copy link
Contributor Author

Yeah, looks good now, thanks!

My comment about squashing the two sorting commits I mentioned above still applies.

I know, I intend to squash all commits into two.

@geoand
Copy link
Contributor

geoand commented Oct 10, 2022

Great, thanks!

@quarkus-bot quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven labels Oct 10, 2022
@manovotn manovotn marked this pull request as ready for review October 10, 2022 12:53
@geoand geoand mentioned this pull request Oct 11, 2022
Merged
@geoand
Copy link
Contributor

geoand commented Oct 11, 2022 •
edited
Loading

The TCK failure also occurred on #27526 so it's unlikely to be related to this change

geoand added a commit to geoand/quarkus that referenced this pull request Oct 11, 2022
@geoand
Fix RESTEasy Reactive testsuite
fbd8fff 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)
@geoand geoand mentioned this pull request Oct 11, 2022
Merged
@geoand
Copy link
Contributor

geoand commented Oct 11, 2022

#28498 fixes the test failure

manovotn and others added 3 commits October 11, 2022 10:53
@manovotn
Revisit the non-standard behavior of ArcContainer#instanceSupplier()
edbc29c 
Modify RR and Undertow processors to add @typed to the resources they discover as beans
@geoand @manovotn
Ensure that providers in RESTEasy Reactive common module aren't disco…
dfb8e54 
…verable.

These providers are registered manually, but previously because
they were annotated with @Provider, they could be inadvertently
registered as application classes in some test cases
(essentially if an actual application class extended them).

Added note about sorting - this note is needed in order to folks debugging in the future
from going down the wrong path and having the TCK fail.

Fix ChunkedResponseTest to be spec compliant.
@manovotn
Rename affected methods and introduce their deprecated variants with …
c5b87cf 
…original syntax to give users time to adapt
@manovotn
Copy link
Contributor Author

Alright, I've rebased to include that commit

@manovotn manovotn requested a review from mkouba October 11, 2022 08:57
mkouba
mkouba approved these changes Oct 11, 2022
View reviewed changes
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.

Looks good. Thanks!

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 11, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 11, 2022

Failing Jobs - Building c5b87cf

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
✖ JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@manovotn manovotn merged commit f7509ba into quarkusio:main Oct 11, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 11, 2022
@manovotn manovotn deleted the issue28115 branch October 11, 2022 14:46
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 11, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this pull request Oct 11, 2022
@geoand @gsmet
Fix RESTEasy Reactive testsuite
8bd4c6a 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)

(cherry picked from commit fbd8fff)
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022
@geoand @igorregis
Fix RESTEasy Reactive testsuite
edba8ec 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022
@geoand @igorregis
Fix RESTEasy Reactive testsuite
b6dc518 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 16, 2022
@geoand @igorregis
Fix RESTEasy Reactive testsuite
7288257 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)
igorregis pushed a commit to igorregis/quarkus that referenced this pull request Oct 17, 2022
@geoand @igorregis
Fix RESTEasy Reactive testsuite
9f17d73 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)
tmihalac pushed a commit to tmihalac/quarkus that referenced this pull request Oct 27, 2022
@geoand @tmihalac
Fix RESTEasy Reactive testsuite
0961691 
The failure showed up in a few PRs
(like quarkusio#28429 and quarkusio#27526)
@geoand geoand mentioned this pull request Oct 31, 2022
Closed
@mkouba mkouba mentioned this pull request Nov 8, 2022
Closed
@mkouba mkouba mentioned this pull request Nov 16, 2022
Closed
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

Assignees
No one assigned
Labels
area/amazon-lambda area/arc Issue related to ARC (dependency injection) area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/funqy area/graphql area/hibernate-orm Hibernate ORM area/infinispan Infinispan area/maven area/metrics area/persistence OBSOLETE, DO NOT USE area/rest area/security area/smallrye area/undertow area/vertx area/websockets release/breaking-change
Projects
None yet
Milestone
2.14.0.CR1
Development

Successfully merging this pull request may close these issues.

Revisit the non-standard behavior of ArcContainer#instanceSupplier()
4 participants
@manovotn @geoand @mkouba @gsmet