ArC: initial support for inactive synthetic beans

[Ladicek]

Fixes #41466

[Ladicek]

This PR is work in progress, hence the draft status. I implemented some (but not all) ideas from #41466 (mostly because I didn't really understand it all and I am not sure how it all fits together) and would like @yrodiere to take it up for a spin before declaring this PR as ready.

[Ladicek]

Just to be sure: the synthetic bean builder (BeanConfigurator, ExtendedBeanConfigurator) gained one new method, checkActive(), to configure the active check function (which is a Runnable). There already is a method to force eager initialization, it's called startup(), so I didn't feel compelled to add another. Eager initialization is skipped for beans that are determined to be inactive.

[yrodiere]

Hey @Ladicek , thanks for working on this.

I'll try to integrate this ASAP, but in the meantime, a few comments:

There already is a method to force eager initialization, it's called startup(), so I didn't feel compelled to add another. Eager initialization is skipped for beans that are determined to be inactive.

Okay, but the requirement was to eagerly initialize only if the bean is active and injected into a user bean. From the issue:

We want startup to fail if the application contains a user bean that gets injected with an active , unconfigured (no JDBC URL) datasource.

I'll have to try, but I suspect without this additional condition, we'll have a few problems, like users discovering only at runtime that their datasource (which they inject and use) didn't start because of missing configuration.

---

Relatedly, one of the main purposes of implementing this in Arc itself (as opposed to doing it by hand in extensions) was this:

We want the failures to include actionable messages:

• The root cause for the problem, specific to each bean: "This datasource is inactive because quarkus.datasource.active is set to false" (DONE)
  • For injected beans, the list of injection points (NOT DONE)

---

I will let you know of my results when I've had a try. Thanks again.

[Ladicek]

Okay, but the requirement was to eagerly initialize only if the bean is active and injected into a user bean. From the issue:

We want startup to fail if the application contains a user bean that gets injected with an active , unconfigured (no JDBC URL) datasource.

I'll have to try, but I suspect without this additional condition, we'll have a few problems, like users discovering only at runtime that their datasource (which they inject and use) didn't start because of missing configuration.

Yeah, so that's one of the things I didn't understand. How can you have an active but unconfigured datasource? If the datasource is unconfigured, it is inactive, like, by definition. No?

I suppose this is an area where more work is required, but I don't really understand what that work is, so I left it for later.

Relatedly, one of the main purposes of implementing this in Arc itself (as opposed to doing it by hand in extensions) was this:

We want the failures to include actionable messages:

• The root cause for the problem, specific to each bean: "This datasource is inactive because quarkus.datasource.active is set to false" (DONE)

  • For injected beans, the list of injection points (NOT DONE)

Actually, also DONE. At least if I understand the requirement correctly. If there's an inactive bean, any attempt to create an instance of that bean (which will happen during startup if the synthetic bean is configured with startup()) will fail with an error whose message includes:

• the message from the exception thrown by checkActive()
• the list of injection points that resolve to that [inactive] bean, if there are any

[yrodiere]

Yeah, so that's one of the things I didn't understand. How can you have an active but unconfigured datasource? If the datasource is unconfigured, it is inactive, like, by definition. No?

I think that's how we ended up defining this... though that's not the current definition. I have a hard time determining if there are cases where we actually want an unconfigured datasource to be considered active (e.g. to get specific errors). I'll I'll have to try.

That being said... if we accept your definition, the requirement becomes this:

We want startup to fail if the application contains a user bean that gets injected with an inactive datasource.

I.e.: since we're now skipping eager initialization for datasources that are inactive, we need to check that no other (user) bean is using these datasources, otherwise the user may start their app forgetting to set the JDBC URL, think everything is fine, and see the app fail on the first request (first time the datasource bean gets accessed).

[Ladicek]

Ouch, yeah, good point. If the bean is not active, initialization on startup() shall be skipped, which is not what we want sometimes (currently: when the bean is injected into a non-synth bean). I'll have to fix that. Thanks for pointing that out!

[Ladicek]

Ouch, yeah, good point. If the bean is not active, initialization on startup() shall be skipped, which is not what we want sometimes (currently: when the bean is injected into a non-synth bean). I'll have to fix that. Thanks for pointing that out!

Done.

[yrodiere]

Thanks!

I this gave a first try here (very incomplete, but it's a start): #41929

I added comments below based on what I noticed.

Right now I'm kind of stuck because whenever there's a bean initialized on startup, and it's inactive, startup fails... even if the bean is not injected in a user bean. See my comment below.

[Ladicek]

Rebased on current main, changed the active check procedure from Runnable to Supplier<ActiveResult> and fixed the problem in StartupBuildSteps.

[yrodiere]

Hey @Ladicek ,

First, sorry for the delay. It's been hard to find time for "features and enhancements" in the past few weeks. But I finally managed to get back to it!

Second, my PR #41929 is in good shape now, and I managed to do everything I needed with your current PR. So, as far as I'm concerned, +1 to merge! I added a few comments, and there are a few unaddressed ones from previous reviews, but I'll let you decide if you want to address them now, later, or never :)

Thanks for all this!

[Ladicek]

Thanks @yrodiere, I'll take a look at the open comments and will do something about them. I should be able to mark this as ready for review in the coming days.

[Ladicek]

Done and marked as ready.

Leaving out for the future:

• an @Active qualifier (or pseudo-qualifier, because it would have to be special-cased)
• ability to use an exception as a cause of ActiveResult

[Ladicek]

Good point. I totally forgot about adding docs. Will do.

[Ladicek]

Hi @yrodiere, in the conversation with @mkouba (see above), we slightly changed the logic in StartupBuildSteps.registerStartupObserver(). Previously, we skipped eager init in case the inactive synthetic bean was injected only into synthetic beans. Now, we skip eager init in case the inactive synthetic bean is injected only into other possibly-inactive synthetic beans. I don't think that should make a difference for you, but could you please check with your branch? Thanks!

[github-actions]

🙈 The PR is closed and the preview is expired.

[yrodiere]

Hi @yrodiere, in the conversation with @mkouba (see above), we slightly changed the logic in StartupBuildSteps.registerStartupObserver(). Previously, we skipped eager init in case the inactive synthetic bean was injected only into synthetic beans. Now, we skip eager init in case the inactive synthetic bean is injected only into other possibly-inactive synthetic beans. I don't think that should make a difference for you, but could you please check with your branch? Thanks!

Thanks for the heads-up. I don't think it will be a problem, and in fact it's something that I wanted initially, but that we had postponed as not being "MVP-worthy", so I'm glad to see it in :) See #41466:

In the future we'll want to trigger initialization on startup even if a bean is only used in other synthetic beans (e.g. a datasource in a Hibernate ORM persistence unit), but that will require more work as we'll want to ignore synthetic consumers that are themselves inactive.

Anyway, I rebased my PR, which is building there: https://github.com/yrodiere/quarkus/actions/runs/11210350803
If that build passes, we're all good.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 30325f2.

✅ 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.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 30325f2.

✅ 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

📦 extensions/infinispan-cache/deployment

✖ io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

• expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

[yrodiere]

Anyway, I rebased my PR, which is building there: https://github.com/yrodiere/quarkus/actions/runs/11210350803
If that build passes, we're all good.

Tests passed 🎉

[Ladicek]

@mkouba / @manovotn, any last comments before I merge? 😁