quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

Published 20 hours ago • verified publisher icon quarkusio

Virtual thread support for smallrye-graphql

Open robp94 opened this issue 6 months ago • 33 comments
trafficstars

Fixes https://github.com/quarkusio/quarkus/issues/35605 ~~Needs a branch from smallrye grapgql https://github.com/smallrye/smallrye-graphql/pull/2287~~

Some questions:

  1. Is there really no context handling needed? As stated here:io.quarkus.virtual.threads.ContextPreservingExecutorService
  2. How to prevent the virtual thread executor from being removed?

Probably we will need more tests, but at least io.quarkus.virtual.graphql.RunOnVirtualThreadTest#testAnnotatedBlockingObject tests for virtual thread and works.

robp94 avatar May 12 '25 11:05 robp94

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

quarkus-bot[bot] avatar May 12 '25 11:05 quarkus-bot[bot]

We'll need @cescoffier for this as well

jmartisk avatar May 12 '25 12:05 jmartisk

Nice !. @jmartisk let's get the smallrye PR in and released, then we can get this builing in CI

phillip-kruger avatar May 13 '25 00:05 phillip-kruger

@cescoffier in the quarkus-virtual-threads-integration-tests-parent this is already configured. See here integration-tests/virtual-threads/pom.xml:107

I added some tests for pinning (Query and Mutation) seems there is no pinning, should we test more variants for pinning?

Do we need to support combinations of @RunOnVirtualThread and @NonBlocking or uni/completionstage?

robp94 avatar May 13 '25 08:05 robp94

Do we need to support combinations of @RunOnVirtualThread and @NonBlocking or uni/completionstage?

No, virtual threads are only for blocking (it could work, but would be quite convoluted). In general, we declare these configurations invalid.

I added some tests for pinning (Query and Mutation) seems there is no pinning, should we test more variants for pinning?

Unfortunately, there is no magic wand here. But it should not block merging this.

cescoffier avatar May 15 '25 08:05 cescoffier

You will need to add

 <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-smallrye-graphql-deployment</artifactId>
            <version>${project.version}</version>
            <type>pom</type>
            <scope>test</scope>
            <exclusions>
                <exclusion>
                    <groupId>*</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
            </exclusions>
        </dependency>

to integration-tests/virtual-threads/graphql-virtual-threads/pom.xml

I'm planning to release SmallRye GraphQL 2.14 with this soon so that the Quarkus side can move forward..

jmartisk avatar May 19 '25 11:05 jmartisk

Btw I am also getting 10 failures in io.quarkus.virtual.graphql.GraphQLThreadTest with java.net.ConnectException: Connection refused, is this supposed to be working already?

jmartisk avatar May 19 '25 11:05 jmartisk

Btw I am also getting 10 failures in io.quarkus.virtual.graphql.GraphQLThreadTest with java.net.ConnectException: Connection refused, is this supposed to be working already?

It should have, but I only ran the tests via intellijs test runner, where they worked. Running via maven, I get the same errors.

I missed one annotation, now it should work.

robp94 avatar May 20 '25 07:05 robp94

I'm planning to release SmallRye GraphQL 2.14 with this soon so that the Quarkus side can move forward.

Any news on smallrye graphql release? @jmartisk

robp94 avatar Jun 03 '25 08:06 robp94

Ah right, thanks for reminding me, I'll do it today

jmartisk avatar Jun 03 '25 08:06 jmartisk

@robp94 2.14.0 should be released now, can you upgrade it in the pom here and unmark as draft?

jmartisk avatar Jun 03 '25 10:06 jmartisk

@jmartisk I upgraded it and marked the pr as ready, so there are still some open questions, like how to make sure the vt executor is not removed.

robp94 avatar Jun 03 '25 12:06 robp94

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0657061bfe4b9a47902464d41fbcdc3fe197518e.

:white_check_mark: 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.

[!WARNING] There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot[bot] avatar Jun 03 '25 12:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5cc15e3c07a3f22b09c158fed0fdd79c0b5d0984.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✖ Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: extensions/smallrye-graphql/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/liquibase/liquibase-mongodb/deployment and 79 more

:package: extensions/smallrye-graphql/runtime

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.12.0:check (check-imports) on project quarkus-smallrye-graphql: Imports are not sorted in /home/runner/_work/quarkus/quarkus/extensions/smallrye-graphql/runtime/src/main/java/io/quarkus/smallrye/graphql/runtime/spi/datafetcher/BlockingHelper.java

quarkus-bot[bot] avatar Jun 03 '25 12:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 3247f6df6afb3a1969a95d7a430747b5e17a5ea9.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✖ Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: integration-tests/virtual-threads/graphql-virtual-threads

:package: integration-tests/virtual-threads/graphql-virtual-threads

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.12.0:check (check-imports) on project quarkus-integration-test-virtual-threads-graphql: Imports are not sorted in /home/runner/_work/quarkus/quarkus/integration-tests/virtual-threads/graphql-virtual-threads/src/test/java/io/quarkus/virtual/graphql/AbstractGraphQLTest.java

quarkus-bot[bot] avatar Jun 03 '25 13:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 3247f6df6afb3a1969a95d7a430747b5e17a5ea9.

:white_check_mark: 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.

quarkus-bot[bot] avatar Jun 03 '25 13:06 quarkus-bot[bot]

🙈 The PR is closed and the preview is expired.

github-actions[bot] avatar Jun 03 '25 13:06 github-actions[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9ad332a351e273712e6f8460d2fde94a1bef9e6d.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✖ Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: integration-tests/virtual-threads/graphql-virtual-threads

:package: integration-tests/virtual-threads/graphql-virtual-threads

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.12.0:check (check-imports) on project quarkus-integration-test-virtual-threads-graphql: Imports are not sorted in /home/runner/_work/quarkus/quarkus/integration-tests/virtual-threads/graphql-virtual-threads/src/test/java/io/quarkus/virtual/graphql/GraphQLThreadTest.java

quarkus-bot[bot] avatar Jun 03 '25 13:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 07e8296ec0a2969410e9efe010a49d3860dd4145.

:white_check_mark: 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.

[!WARNING] There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot[bot] avatar Jun 03 '25 14:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 4b173d034dc9861deb0c63f4bedab80a517b9bb6.

:white_check_mark: 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.

[!WARNING] There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot[bot] avatar Jun 03 '25 15:06 quarkus-bot[bot]

@jmartisk I upgraded it and marked the pr as ready, so there are still some open questions, like how to make sure the vt executor is not removed.

Looking at other extensions and how they do it, it seems they use VirtualThreadsRecorder.getCurrent() instead of CDI to obtain the executor. @cescoffier, should smallrye-graphql do it too, or should we add something to prevent removing the bean, like, for example, mark it unremovable if we detect at least one @RunOnVirtualThread annotation?

jmartisk avatar Jun 04 '25 06:06 jmartisk

@cescoffier which variant from @jmartisk suggestions should we use?

robp94 avatar Jun 12 '25 12:06 robp94

As smallrye cannot use the Quarkus executor, we need to adapt the extension to make the executor in removable if the annotation is found on a graphql method. You don't need to check for the Java version, as the executor falls back automatically.

cescoffier avatar Jun 12 '25 12:06 cescoffier

Do we need both? Or only one of them? Is this the correct way, or should we do something else?

            unremovableBeans.produce(UnremovableBeanBuildItem
                    .beanTypes(DotName.createSimple("io.quarkus.virtual.threads.ContextPreservingExecutorService")));
            unremovableBeans.produce(UnremovableBeanBuildItem
                    .beanTypes(DotName.createSimple("io.quarkus.virtual.threads.DelegatingExecutorService")));

robp94 avatar Jun 12 '25 16:06 robp94

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✖ Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: integration-tests/virtual-threads/graphql-virtual-threads

:package: integration-tests/virtual-threads/graphql-virtual-threads

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.12.0:check (check-imports) on project quarkus-integration-test-virtual-threads-graphql: Imports are not sorted in /home/runner/_work/quarkus/quarkus/integration-tests/virtual-threads/graphql-virtual-threads/src/test/java/io/quarkus/virtual/graphql/RunOnVirtualThreadTest.java

quarkus-bot[bot] avatar Jun 12 '25 16:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit c1ac1bb980bcbf30df78fa330d23f02af7d89b21.

:white_check_mark: 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.

quarkus-bot[bot] avatar Jun 12 '25 17:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 54ea74523045053a9b0f21451f71482af2e7e325.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✖ Initial JDK 17 Build Build Failures Logs Raw logs :mag:

You can consult the Develocity build scans.

Failures

:gear: Initial JDK 17 Build #

- Failing: integration-tests/virtual-threads/graphql-virtual-threads

:package: integration-tests/virtual-threads/graphql-virtual-threads

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.12.0:check (check-imports) on project quarkus-integration-test-virtual-threads-graphql: Imports are not sorted in /home/runner/_work/quarkus/quarkus/integration-tests/virtual-threads/graphql-virtual-threads/src/test/java/io/quarkus/virtual/graphql/RunOnVirtualThreadTest.java

quarkus-bot[bot] avatar Jun 12 '25 18:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 54ea74523045053a9b0f21451f71482af2e7e325.

:white_check_mark: 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.

quarkus-bot[bot] avatar Jun 12 '25 19:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 39e4598dfa429b6adc0328db313b8b4429d101cf.

:white_check_mark: 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.

[!WARNING] There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot[bot] avatar Jun 15 '25 13:06 quarkus-bot[bot]

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit a719272cb1fba18abb7b3571f0d2e87ddc29728a.

:white_check_mark: 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.

[!WARNING] There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot[bot] avatar Jun 15 '25 18:06 quarkus-bot[bot]