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

Run authentication request blocking tasks on Vert.x duplicated context #34963

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Jul 24, 2023

fixes: #34912

Newly introduced blocking executor is introduced as a bean because there are 2 other places in security code where I' like to use this new blocking executor (io.quarkus.vertx.http.runtime.security.AbstractHttpAuthorizer#CONTEXT and io.quarkus.oidc.runtime.OidcRecorder#setup), but it is not necessary change to fix this very issue. I will propose it in a separate PR if this one will be accepted. This way, it will be clear what was necessary change in order to fix this issue.

@michalvavrik michalvavrik marked this pull request as ready for review July 24, 2023 16:02
@michalvavrik
Copy link
Member Author

Hey @sberyozkin , please let's see how CI is doing before you review this PR, thanks. IMHO it should be fine, but I only run test that I added, so let's safe your time. I meant to have this as a draft and run CI, but it looks like drafts don't run full CI anymore (I think workflows wrote skipped).

@michalvavrik
Copy link
Member Author

michalvavrik commented Jul 24, 2023

Hello @cescoffier , could you kindly have a look whether this change is correct in regards of duplicated context and running blocking tasks, please? Here is short context on why I made this change #34912 (comment), though I didn't dig deeper on Vert.X / Netty that causes linked issue as this TODO

//TODO: should we be using vert.x blocking tasks here? We really should only have a single thread pool
is something I had in mind for some time now.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/rr-smallrye-jwt-fix-ex-mappers-for-http2 branch from 58233e2 to ef034bc Compare July 25, 2023 06:23
@michalvavrik
Copy link
Member Author

IMHO failures are unrelated, but I rebased on main to make sure it is just flaky tests.

@michalvavrik
Copy link
Member Author

michalvavrik commented Jul 25, 2023

One of failures sounds suspicious by name VertxBlockingMutinyTest in grpc-hibernate integration module, but I can't reproduce it locally and blocking handler changed here is not used there, so I believe it is also unrelated, but let's see whether CI can confirm that I didn't miss something.

@cescoffier cescoffier changed the title Run authentication request blocking tasks on Vert.X duplicated context Run authentication request blocking tasks on Vert.x duplicated context Jul 25, 2023
@sberyozkin
Copy link
Member

@michalvavrik Thanks, looks nice. But I'd like to ask to wait for another day or so for @stuartwdouglas have a quick look as well, cheers

@sberyozkin
Copy link
Member

And for Clement too :-)

@michalvavrik
Copy link
Member Author

michalvavrik commented Jul 25, 2023

Quarkus CI / JVM Tests - JDK 17 (pull_request) failed over grpc-hibernate. I can prove it is flaky test: here I found CI run that also failed https://github.com/quarkusio/quarkus/actions/runs/5648131944 over this test for this PR #34961 which is completely unrelated.

@michalvavrik
Copy link
Member Author

Quarkus CI / JVM Tests - JDK 11 (pull_request) and Quarkus CI / JVM Tests - JDK 17 Windows (pull_request) were canceled because there is 6 hours Github CI limit for runs, it is not consequence of this PR.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 25, 2023

Failing Jobs - Building ef034bc

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 17 Build Failures Logs Raw logs
JVM Tests - JDK 17 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 19
✔️ Maven Tests - JDK 11
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/grpc-hibernate 

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.VertxBlockingMutinyTest.shouldAddItems - More details - Source on GitHub

io.smallrye.mutiny.CompositeException: 
Multiple exceptions caught:
	[Exception 0] io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request

@sberyozkin
Copy link
Member

grpc-hibernate is flaky.

@sberyozkin
Copy link
Member

With Clement approving I think it is really ready to go Michal, but lets wait for another day in case Stuart will have something to comment about

@sberyozkin sberyozkin merged commit 0c751fe into quarkusio:main Jul 27, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 27, 2023
@michalvavrik michalvavrik deleted the feature/rr-smallrye-jwt-fix-ex-mappers-for-http2 branch July 27, 2023 12:26
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.

Failed auth with blocking authentication & a custom authentication failure mapper cause HTTP/2 error
3 participants