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.

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

Quarkus REST - reuse CDI request context if it exists #40408

Merged
merged 2 commits into from
May 3, 2024

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented May 2, 2024

Related to: #40307

Allows reuse of existing CDI req. context if present and monitors if req. context was activated from within AbstractResteasyReactiveContext. Only performs cleanup if the context was initialized here.

@manovotn
Copy link
Contributor Author

manovotn commented May 2, 2024

I'll turn this into a regular PR to get full test coverage.

@michalvavrik feel free to add the tests you mentioned straight into this PR.

@michalvavrik
Copy link
Contributor

michalvavrik commented May 2, 2024

Hello @manovotn , thank you for prompt debugging, feedback and fix. I have prepared reproducer inside OpenAPI IT module (because that is where Quarkus REST and Reactive Routes already are, no better place I could find).

I cannot push it to your PR, please just pickup the commit, refactor it to your liking and use it.

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.

I do agree with the idea (and it's consistent with what other extensions are doing; e.g. reactive routes) but I don't feel qualified to comment on the implementation 🤷 .

@manovotn
Copy link
Contributor Author

manovotn commented May 2, 2024

Hello @manovotn , thank you for prompt debugging, feedback and fix. I have prepared reproducer inside OpenAPI IT module (because that is where Quarkus REST and Reactive Routes already are, no better place I could find).

* [125453e](https://github.com/quarkusio/quarkus/commit/125453ecd23905b5da2664c82b987289f0a1f0af)

I cannot push it to your PR, please just pickup the commit, refactor it to your liking and use it.

Ah, I didn't realize.
Cherry picked the commit into this PR so we should be good once CI gives us green light 👍

@quarkus-bot

This comment has been minimized.

@manovotn
Copy link
Contributor Author

manovotn commented May 2, 2024

Failures are related. Those are SR CP tests asserting destruction of beans in a no-propagation scenario that now doesn't happen correctly.
I will debug tomorrow but my prime suspect is that I made the destruction logic conditional whereas before we did it at all times which seemed weird but this might have been the reason.

@quarkus-bot
Copy link

quarkus-bot bot commented May 3, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 864be71.

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

@michalvavrik
Copy link
Contributor

@manovotn please change PR desc so that it closes #40307, thanks.

@manovotn
Copy link
Contributor Author

manovotn commented May 3, 2024

@manovotn please change PR desc so that it closes #40307, thanks.

It is already linked to that issue and will autoclose it on merge (see the Development section of the PR)

@manovotn manovotn merged commit 8832fa8 into quarkusio:main May 3, 2024
45 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone May 3, 2024
@manovotn manovotn deleted the issue40307 branch May 3, 2024 13:53
@gsmet
Copy link
Member

gsmet commented May 10, 2024

My understanding is that this should be backported as far back as 3.8 as the original report is that 3.8.4 introduces the issue?

@gsmet
Copy link
Member

gsmet commented May 10, 2024

I added the backport but please I'd like someone to confirm :).

@geoand
Copy link
Contributor

geoand commented May 10, 2024

Yeah, that makes sense

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.

WebSocket + Role-based authentication stopped working with Quarkus 3.9.x: Security Identity is not available
5 participants