CredentialsProvider API should be asynchronous #50047

tsegismont · 2025-09-13T17:13:30Z

The synchronous method is deprecated and is invoked by the asynchronous variant.

Extensions must be updated to invoke the asynchronous variant.

Closes: CredentialsProvider API should be asynchronous #32021

gastaldi · 2025-09-14T13:19:27Z

CI errors seem to be related?

cescoffier · 2025-09-14T14:07:10Z

I like the change (and actually need it for the mailer), but I think a few usages have been overlooked.

sberyozkin · 2025-09-14T14:19:45Z

extensions/credentials/runtime/src/main/java/io/quarkus/credentials/CredentialsProvider.java

+     */
+    default Uni<Map<String, String>> getCredentialsAsync(String credentialsProviderName) {
+        return Uni.createFrom().item(() -> getCredentials(credentialsProviderName))
+                .runSubscriptionOn(Infrastructure.getDefaultExecutor());


I'm not sure it is correct, this is not where the subscription should be enabled, as the consumer of the api may be using another pool, this is probably what is causing the test failures...

I believe the default implementation should be return Uni.createFrom().item(getCredentials(credentialsProviderName));

I'm not sure what you mean with subscription enabled.

Subscription does not happen here. runSubscriptionOn operator configures the Uni so that, when the subscription happens, it happens on the default executor (a Quarkus worker thread).

Since existing getCrendentials implementation are synchronous, it is safer to invoke the method on a worker thread.

@tsegismont Sorry, I meant to say, that this is not a concern of this api method. It is a problem for the consumers of this api.

I'm 100% sure tests will pass without it. Otherwise it becomes a breaking change unfortunately.

To avoid some embarassment, let me say I'm quite sure it will be fixed without it, rather than quoting 100% 🙂.
I support this PR

The problem was not with the default implementation of getCredentialsAsync, but with the synchronous nature of io.quarkus.oidc.common.runtime.OidcCommonUtils#fromCredentialsProvider even though this method is invoked on an event loop thread.

I reverted the change in this method and added a comment. Ideally, the problem would be addressed in a follow-up PR, but I'm not knowledgeable with this area. I could help with a PR review though

Thanks @tsegismont.
When I get to that, I'll definitely cc you.
IMHO though, it would still be better to avoid setting a particular thread pool in the default implementation, since its role is to avoid breaking existing code, not to offer an actual async implementation.
Custom extensions or code that has no async CP requirements or immediate capabilites may have no other option but to keep the deprecated method ref.
IMHO your original update to OidcCommonUtils should've worked as you typed it.

In any case, thanks for the PR

tsegismont · 2025-09-15T08:27:58Z

I like the change (and actually need it for the mailer), but I think a few usages have been overlooked.

Thanks for the feedback, I'm looking into the failures now

yrodiere · 2025-09-15T09:31:29Z

extensions/credentials/runtime/src/main/java/io/quarkus/credentials/CredentialsProvider.java

+    @Deprecated
+    default Map<String, String> getCredentials(String credentialsProviderName) {


I'm not sure it's a great idea to deprecate the blocking variant considering some are allergic to reactive and would have a blocking implementation anyway.

Keeping both methods would be confusing though. I suppose the alternative would be to have two interfaces, and creating adapters depending on what the consumer that calls the provider prefers (blocking or reactive)?

Anyway, I suppose you discussed this with @cescoffier , so +0 from me.

The discussion started from #31873 (comment)

The problem we're trying to address is twofold:

there are implementations of CredentialsProvider that rely on async code, and they must block until the result is available

there are extensions that call the currently synchronous contract of CredentialsProvider from an event loop thread (which is bad because it can block the event loop)

Having a single async method allows to solve both problems, but it is true that some people prefer to stay away from reactive. A separate interface (BlockingCredentialsProvider ?) could do the trick. Are there other places in Quarkus where user-provided code can be invoked from an extension, possibly from an event loop? I'm asking because if the problem has been solved elsewhere we can inspire from the solution.

Thoughts @cescoffier ?

@tsegismont Hi,

Having a single async method allows to solve both problems, but it is true that some people prefer to stay away from reactive. A separate interface (BlockingCredentialsProvider ?) could do the trick.

It is certainly not me :-), I'll have no problems updating the OIDC code (not immediately, but I opened an issue with CC to you), I think your new interface method is perfect, my only reservation is that if someone has a blocking call working for whatever reason, then, ideally, the same blocking call expressed in Uni await().indefinitely() terms should also work to allow a simple move away from the deprecated method.

What if the the consumer of the API already running on a ManagedExecutor worker thread, in a blocking mode, where a custom CP can have some request scoped bean injected...

In any case, apologies, I'm not really an expert in how to link various threads together across all the chain call, so please ignore me if I talk some nonsense :-)

I'm going quiet now, as I'm in Out of office mode
Thanks

Sorry, 1 more comment, I'm not 100% sure one more interface is necessary, as we discussed it before with Clement, Roberto, since this interface is not part of the Quarkus Configuration system. Ideally, some dynamism would be added to the ConfigSource or other smallrye-config specific provider if refreshing secrets is necessary...

Cheers

My point was that users would not necessarily want to write reactive code for this -- especially if their credentials retrieval code is blocking anyway.

Now if we consider this is more of an SPI, where implementors would be extension maintainers (e.g. Vault, AWS, etc.) and that users have no business writing their own... Then I agree reactive-only is fine.

My point was that users would not necessarily want to write reactive code for this -- especially if their credentials retrieval code is blocking anyway.

Right

Now if we consider this is more of an SPI, where implementors would be extension maintainers (e.g. Vault, AWS, etc.) and that users have no business writing their own... Then I agree reactive-only is fine.

In most cases, users don't have to implement their own, but it happens https://quarkus.io/guides/credentials-provider#custom-credentials-provider

I would not deprecate, but give the choice.

sberyozkin

Thanks @tsegismont

geoand · 2025-09-18T06:50:46Z

@yrodiere any last comments?

yrodiere · 2025-09-18T07:25:29Z

@yrodiere any last comments?

Still this for me:

Anyway, I suppose you discussed this with @cescoffier , so +0 from me.

cescoffier · 2025-09-18T09:09:09Z

So, no I discovered the PR.
As I said as comment, I would not deprecate the blocking API, but implement one on top of the other (using a default method).

tsegismont · 2025-09-18T16:39:16Z

So, no I discovered the PR. As I said as comment, I would not deprecate the blocking API, but implement one on top of the other (using a default method).

The implementation does exactly this, so we're good on that front.

No problem, I'll remove the deprecation part

tsegismont · 2025-09-18T16:58:14Z

@cescoffier PTAL

cescoffier

For me the question is how the credential provider is intended to be implemented and used.

For the usage, generalizing async and awaiting if needed is accepetable.
However, to do that, the provider needs to be implemented using an async API.

I would not deprecate the blocking API, but instead try to find a way to have both, with one using the other if needed. We cool imagine adding a new interface for async credential providers if needed.

For blocking (or synchronous) async provider, extensions should be careful and call them from a different thread if they are on the event loop.

tsegismont · 2025-09-19T09:31:48Z

@cescoffier in the last commit, I removed the deprecation and the synchronous method is invoked from the async one (as discussed in #32021 (comment))

So, if I understand correctly, you changed your mind and would prefer separate interfaces for async/sync credential providers?

It's fine with me, I just want to make sure we agree on the desired implementation before changing the PR.

Closes quarkusio#32021 By default, the synchronous method is invoked by the asynchronous variant on a worker thread. Extensions must be updated to invoke the asynchronous variant.

quarkus-bot · 2025-09-19T14:54:26Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 1c46002.

✅ 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

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/devmode

❌ io.quarkus.test.devui.DevUIGrpcSmokeTest.testTestService - History

Too many recursions, message not returned for id [1277288190] - java.lang.RuntimeException

java.lang.RuntimeException: Too many recursions, message not returned for id [1277288190]
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:175)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:178)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:178)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:178)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:178)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:178)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:178)

📦 integration-tests/maven

❌ io.quarkus.maven.it.DevMojoIT.testClassLoaderLinkageError - History

Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1160)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1129)
	at io.quarkus.test.devmode.util.DevModeClient.getHttpResponse(DevModeClient.java:164)
	at io.quarkus.test.devmode.util.DevModeClient.getHttpResponse(DevModeClient.java:155)

❌ io.quarkus.maven.it.DevMojoIT.testClassLoaderLinkageError - History

Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1160)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1129)
	at io.quarkus.test.devmode.util.DevModeClient.getHttpResponse(DevModeClient.java:164)
	at io.quarkus.test.devmode.util.DevModeClient.getHttpResponse(DevModeClient.java:155)

cescoffier · 2025-09-21T15:24:51Z

Can we have both in the same interface?

tsegismont · 2025-09-22T12:35:31Z

Can we have both in the same interface?

Yes, this is what this PR does

tsegismont force-pushed the async-credentials-api branch from 01ba773 to 54e8cad Compare September 13, 2025 17:13

quarkus-bot bot added area/agroal area/mongodb area/oidc area/reactive-messaging area/reactive-sql-clients area/smallrye area/vertx labels Sep 13, 2025

tsegismont requested a review from cescoffier September 13, 2025 17:14

This comment has been minimized.

Sign in to view

gastaldi requested a review from sberyozkin September 14, 2025 13:19

sberyozkin reviewed Sep 14, 2025

View reviewed changes

tsegismont force-pushed the async-credentials-api branch from 54e8cad to 4e62892 Compare September 15, 2025 09:21

geoand requested a review from yrodiere September 15, 2025 09:23

yrodiere reviewed Sep 15, 2025

View reviewed changes

sberyozkin approved these changes Sep 15, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Sep 15, 2025

tsegismont mentioned this pull request Sep 18, 2025

Credentials provider for Infinispan clients #50109

Open

tsegismont force-pushed the async-credentials-api branch from 4e62892 to 04136cd Compare September 18, 2025 16:57

This comment has been minimized.

Sign in to view

cescoffier reviewed Sep 19, 2025

View reviewed changes

tsegismont force-pushed the async-credentials-api branch from 04136cd to 9097171 Compare September 19, 2025 07:41

This comment has been minimized.

Sign in to view

CredentialsProvider API should be asynchronous

1c46002

Closes quarkusio#32021 By default, the synchronous method is invoked by the asynchronous variant on a worker thread. Extensions must be updated to invoke the asynchronous variant.

tsegismont force-pushed the async-credentials-api branch from 9097171 to 1c46002 Compare September 19, 2025 12:28

		@Deprecated
		default Map<String, String> getCredentials(String credentialsProviderName) {

CredentialsProvider API should be asynchronous #50047

Are you sure you want to change the base?

CredentialsProvider API should be asynchronous #50047

Uh oh!

Conversation

tsegismont commented Sep 13, 2025 • edited by geoand Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

gastaldi commented Sep 14, 2025

Uh oh!

cescoffier commented Sep 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsegismont commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsegismont Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sberyozkin Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sberyozkin left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

geoand commented Sep 18, 2025

Uh oh!

yrodiere commented Sep 18, 2025

Uh oh!

cescoffier commented Sep 18, 2025

Uh oh!

tsegismont commented Sep 18, 2025

Uh oh!

tsegismont commented Sep 18, 2025

Uh oh!

This comment has been minimized.

cescoffier left a comment

Choose a reason for hiding this comment

Uh oh!

tsegismont commented Sep 19, 2025

Uh oh!

This comment has been minimized.

quarkus-bot bot commented Sep 19, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/devmode

📦 integration-tests/maven

Uh oh!

cescoffier commented Sep 21, 2025

Uh oh!

tsegismont commented Sep 22, 2025

Uh oh!

Uh oh!

tsegismont commented Sep 13, 2025 •

edited by geoand

Loading

tsegismont commented Sep 15, 2025 •

edited

Loading

tsegismont Sep 15, 2025 •

edited

Loading

sberyozkin Sep 15, 2025 •

edited

Loading

quarkus-bot bot commented Sep 19, 2025 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`