Skip to content

Fixes request context lost in Infinispan cache get/getasync #48200

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

Merged
merged 1 commit into from
Jun 15, 2025

Conversation

karesti
Copy link
Member

@karesti karesti commented Jun 3, 2025

This comment was marked as resolved.

@karesti karesti requested a review from wburns June 3, 2025 16:07

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jun 4, 2025

My high level question is why are things being done with Context Propagation instead of the way that io.quarkus.cache.redis.runtime.RedisCacheImpl does them?

@geoand geoand changed the title [#48196] Fixes request context lost in Infinispan cache get/getasync Fixes request context lost in Infinispan cache get/getasync Jun 4, 2025
@karesti
Copy link
Member Author

karesti commented Jun 4, 2025

@geoand when I looked at the code of redis, I was under the impression that is because the client is the quarkus vert.x client, everything was working "automatically" comparing to the infinispan client. My personal issue is that I lack a bit of deep underlying knowledge at this point

@geoand
Copy link
Contributor

geoand commented Jun 4, 2025

Okay, I was hoping that the Infinispan client had a Vert.x version... I know we can't do that now, but at some point we really should look into that.

@karesti karesti force-pushed the fix-48196 branch 2 times, most recently from c6b51fb to 65029b2 Compare June 10, 2025 12:38

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@karesti
Copy link
Member Author

karesti commented Jun 10, 2025

@cescoffier does this seem good ?

@cescoffier
Copy link
Member

Should we use the duplicated context instead of context propagation?
I'm not sure if it would fix the issue, but I think it's worth trying.

In this case, you would need to create an executor at the beginning of each method:

var context = Vertx.currentContext();
var executor = r -> {
  if (context == null) r.run();
  else context.runOnContext(x -> r.run());
}

(Avoid the lambdas in the real code)

Then, before returning the Uni:

.emitOn(executor);

@karesti karesti force-pushed the fix-48196 branch 3 times, most recently from de25fa3 to 868029d Compare June 10, 2025 16:26
@karesti
Copy link
Member Author

karesti commented Jun 10, 2025

Should we use the duplicated context instead of context propagation? I'm not sure if it would fix the issue, but I think it's worth trying.

In this case, you would need to create an executor at the beginning of each method:

var context = Vertx.currentContext();
var executor = r -> {
  if (context == null) r.run();
  else context.runOnContext(x -> r.run());
}

(Avoid the lambdas in the real code)

Then, before returning the Uni:

.emitOn(executor);

I tested and runned into the Suppressed: jakarta.enterprise.context.ContextNotActiveException: RequestScoped context was not active when trying to obtain a bean instance for a client proxy of CLASS bean [class=io.quarkus.it.cache.infinispan.ClientRequestService, id=VcMhAzqfwnRsjHRm80Y_wUxTmRc]
1653

This comment has been minimized.

@karesti karesti force-pushed the fix-48196 branch 2 times, most recently from 98b9668 to 8c17e12 Compare June 12, 2025 13:48

This comment has been minimized.

@karesti
Copy link
Member Author

karesti commented Jun 12, 2025

@cescoffier thanks for all your insights and help, let me know now

This comment has been minimized.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

It would have been nice to replace the lambdas

@karesti
Copy link
Member Author

karesti commented Jun 13, 2025

@cescoffier I removed the lambdas

Copy link

quarkus-bot bot commented Jun 13, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9936f54.

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

@cescoffier cescoffier merged commit 55c6021 into quarkusio:main Jun 15, 2025
25 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.25 - main milestone Jun 15, 2025
@gsmet gsmet modified the milestones: 3.25 - main, 3.24.0 Jun 18, 2025
@gsmet gsmet modified the milestones: 3.24.0, 3.23.4 Jun 18, 2025
@karesti karesti deleted the fix-48196 branch June 18, 2025 08:25
@cescoffier
Copy link
Member

Added the backport-3.20 label to investigate if it's possible or not.
(I know that 3.20 is frozen, so it will be in the next one)

CC @karesti

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinispan cache does not propagate request context
5 participants