Use unordered executeBlocking for GraphQL blocking resolvers#54927
Use unordered executeBlocking for GraphQL blocking resolvers#54927phillip-kruger wants to merge 1 commit into
Conversation
bea0293 to
4a3a7b1
Compare
|
🎊 PR Preview 77c9a79 has been successfully built and deployed to https://quarkus-pr-main-54927-preview.surge.sh/version/main/guides/
|
|
Hello @phillip-kruger ! Thank you so much for looking into this. Only some hours after my comment. I am a senior java developer with more than three decades of coding experience. But what you are writing up there sounds like magic even to me. So once again: KUDOs and I am looking forward to the next quarkus and/GraphQL fix version! 👍 |
|
@Doogiemuc what would help is if you can build against this branch and confirm if this solves the issue for you |
|
"build against this branch" Does that mean I would need to build quarkus from source? I'd assume this is a big task that would take me a day just to setup all dependencies. Or do I miss something and there is an easy way? |
|
@Doogiemuc it's fairly easy:
Then do https://github.com/quarkusio/quarkus/blob/main/CONTRIBUTING.md#building-main Once a local version is build you can use that in your app: |
4a3a7b1 to
15fcb49
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cescoffier
left a comment
There was a problem hiding this comment.
I'm ok with this change. I made some comment on the test.
@jponge We had to modify this code heavily for Vert.x 5 - expect the next merge to be chaotic.
|
So I'll need a bit more of an explanation here, how is this different from #29306 which I submitted in the past, but it turned out to break Hibernate? |
|
Hi @jmartisk, your PR #29306 was the right fix, the only reason it broke Hibernate was that RequestScopedSessionHolder and RequestScopedStatelessSessionHolder use a HashMap internally, which isn't thread-safe when multiple blocking resolvers run concurrently within the same request. I've now added that fix in this PR as well (switched both to ConcurrentHashMap). So in short: this PR = your original ordered=false change + the ConcurrentHashMap fix for the Hibernate session holders that was the actual root cause of the breakage. |
…urrentHashMap for request-scoped Hibernate session holders
d86718d to
42ed240
Compare
|
I also proposed the switch to the |
Status for workflow
|
Indeed. If multiple threads are hitting the same Whatever threads are involved here need to either:
EDIT: though if it's supposed to be atomic/consistent DB accesses, as Sanne suggested you would really want to use a single transaction for all this, which would imply a single thread. |
|
It's not quite fair to state "it would break Hibernate". Hibernate could in theory do this, but it's enforcing safety by design - and efficiency I should add: if multiple operations need to happen on the DB, it's a waste of resources to initiate multiple transactional contexts and let them "figure out" any data race condition. It's also extremely error prone as exact impact on the final data representation will become timing dependent, becoming error prone and introducing difficult to reproduce interleavings. Make an appropriate plan at the client side, decide what it is exactly that needs to happen and send a single coherent plan to the database. Hibernate would help to define such a consistent plan. |
Status for workflow
|

When a blocking GraphQL resolver calls a reactive Vert.x client — such as the imperative Mailer — the resolver can deadlock. The root cause is that
BlockingHelper.runBlocking()dispatches blocking resolvers viaexecuteBlockingwith aRequestScopedTaskQueuethat serializes tasks per-request. The reactive client's event-loop callbacks can't dispatch while the ordered task slot is held, creating a circular wait.This removes the
RequestScopedTaskQueuemechanism and always usesexecuteBlocking(callable, false)(unordered). This is safe because:AsyncSerialExecutionStrategy— confirmed by @jmartiskNot a revert of #54372
This is not a revert of the
RequestScopedTaskQueuechange (PR #54372). Before that PR, the code usedvc.executeBlocking(callable)with no explicit ordering parameter, which defaulted toordered=truein Vert.x 4 — globally serializing all requests sharing an event loop (the cause of #54361). This PR usesexecuteBlocking(callable, false)— explicitly unordered — which is a state that never existed before:executeBlocking(callable)— implicittrueexecuteBlocking(callable, queue)executeBlocking(callable, false)Includes a regression test that sends mail via the imperative Mailer from a
@BlockingGraphQL resolver using a real (non-mock) SMTP connection.Fixes #29141
Fixes #36215