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

Fix and reenable SimpleContextPropagationTest.testArcMEContextPropagationDisabled #28129

Closed
gsmet opened this issue Sep 21, 2022 · 8 comments · Fixed by #28367
Closed

Fix and reenable SimpleContextPropagationTest.testArcMEContextPropagationDisabled #28129

gsmet opened this issue Sep 21, 2022 · 8 comments · Fixed by #28367

Comments

@gsmet
Copy link
Member

gsmet commented Sep 21, 2022

It has been disabled here because it was flaky: #28127 .

/cc @mkouba @FroMage

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 21, 2022

/cc @FroMage, @manovotn

@manovotn
Copy link
Contributor

@gsmet got link to some PRs that failed this?
I have been trying to reproduce this with a loop execution locally but so far not a single failure.

BTW this is the same test that we tried to fix in #27284
Although back then I think I saw at least some failures locally...

@gsmet
Copy link
Member Author

gsmet commented Sep 21, 2022

Yes, here for instance: #28121

@manovotn
Copy link
Contributor

I spent some fair tome debugging this yesterday.
From my findings, I was never able to reproduce the failure when running the whole test class.
However, if you run this one test directly, you can (at least mostly) get the failure.

I could see that in case of failure the CP actually behaves correctly but the problem is with the original req. context inside plain @Get method - this one doesn't get terminated at all.
Now, these test use old RE Classic with undertow beneath; I am not sure if it is intended or if we could just rewrite them to use RE reactive. Note that some of the tests make use of io.reactivex.Single and io.smallrye.mutiny.Multi as the endpoint method's return value and I have no clue if this is legit in RE reactive.

Maybe @geoand would know if rewriting this to RE reactive makes sense?
It'd probably be more fruitful than trying to pinpoint the failure for RE classic and undertow which are (as far as I know) deprecated anyway?

@geoand
Copy link
Contributor

geoand commented Sep 22, 2022

Yes, it makes total sense. The combination of using reactive types with RESTEasy Classic is just too problematic (and is one of the reasons why we did RR in the first place).

@gsmet
Copy link
Member Author

gsmet commented Sep 22, 2022

BTW, I'm wondering if we could get rid of io.reactivex.Single now? @cescoffier WDYT?

@cescoffier
Copy link
Member

@gsmet yes, probably time to remove the RX java 2 support

@geoand
Copy link
Contributor

geoand commented Sep 22, 2022

Definitely +1 for that

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