Use FileSystemStaticHandler for all webjars #23030

Postremus · 2022-01-19T19:24:19Z

The changes in this PR prevent copying all static resources needed for webjars (smallrye-health, swaggerui, graphql), and serves them directly from the jar instead.

I also replaced the extension specific handlers (e.g. SwaggerUiNotFoundHandler, SwaggerUiStaticHandler) with general purpose implementation (WebJarNotFoundHandler, WebJarStaticHandler) in vertx-http.

Same idea as in #22525.

This saves about 30ms of dev mode startup time.
Relates to #21552

quarkus-bot · 2022-01-19T23:38:51Z

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

Failing Jobs - Building `ae78856`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 2 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario1 - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

gsmet

Looks good to me and a nice cleanup.

I just added a small open question.

gsmet · 2022-01-20T12:32:52Z

.../swagger-ui/deployment/src/main/java/io/quarkus/swaggerui/deployment/SwaggerUiProcessor.java

@@ -136,6 +136,7 @@ public void getSwaggerUiFinalDestination(
            byte[] indexHtmlContent = generateIndexHtml(openApiPath, swaggerUiPath, swaggerUiConfig, indexRootPathBuildItem);
            webJarBuildProducer.produce(
                    WebJarBuildItem.builder().artifactKey(SWAGGER_UI_WEBJAR_ARTIFACT_KEY) //
+                            .onlyCopyNonArtifactFiles(true)


Why this change? Shouldn't it be the default if we generalize it?

BTW, it's an open question, you're the one with the most knowledge of this stuff so I'm curious to hear your opinion.

Setting onlyCopyNonArtifactFiles to default as true, could break quarkiverse members that have already updated to the new webjar infrastructure with 2.7.0.CR1.

I (personally) don't believe a .Final should intentionally break extensions.
WDYT?

Well, if making it the default makes sense, I prefer we do it now rather than after because it will be far more of a pain to change it later.

Now, I don't know if it makes sense to make it the default - I will let you decide what makes more sense on this - but, if it does, let's do it now.

Yep, defaulting to true makes sense, and is easy enough to change if one needs to.
Done.

Postremus · 2022-01-22T20:38:24Z

I fixed the vert.x 4.2.4 deprecations in WebJarStaticHandler, and disallowed rootfilesystemaccess.
This handler only needs to serve from META-INF anyway.

/cc @cescoffier

quarkus-bot · 2022-01-23T22:43:19Z

Failing Jobs - Building `4bbaa52`

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.CustomQuarkusDevModeConfigurationTest.main line 13 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

Postremus · 2022-01-24T12:55:25Z

Test failures in CustomQuarkusDevModeConfigurationTest do not look related

gsmet · 2022-01-24T18:00:34Z

I'm testing this locally and will merge if things are OK.

gsmet · 2022-01-24T18:06:15Z

Thanks @Postremus , very nice work!

gsmet · 2022-01-27T13:51:49Z

...aphql/runtime/src/main/java/io/quarkus/smallrye/graphql/runtime/SmallRyeGraphQLRecorder.java

-            return new SmallRyeGraphQLStaticHandler(graphqlUiFinalDestination, graphqlUiPath);
+            WebJarStaticHandler handler = new WebJarStaticHandler(graphqlUiFinalDestination, graphqlUiPath,
+                    webRootConfigurations);
+            shutdownContext.addShutdownTask(new ShutdownContext.CloseRunnable(handler));


@mkouba @cescoffier I was going back to this patch and was wondering why we needed to do that? Shouldn't the handler be closed by Vert.x itself?

https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/devmode/FileSystemStaticHandler.java#L166

This is the handler in question which should be closed.
At first, I also hopped that a instance of Closeable is automaticly closed.
I set a breakpoint in the close method. Never got invoked though.

This is why I decided to add the shutdownhandlers.

Is there any better / automatic way with Vert.x?

Ah OK, I missed you specifically implemented Closeable. Maybe @mkouba will have some ideas about how we could improve it and close Closeable handlers automatically.
If not, that's not a biggy.

I was going through this patch again to move Quarkus GitHub App to this new pattern.

quarkus-bot bot added area/graphql area/health area/smallrye area/swagger-ui area/vertx labels Jan 19, 2022

Postremus requested review from gsmet and phillip-kruger and removed request for gsmet January 19, 2022 19:24

Postremus force-pushed the webjar-serve-as-jar branch from 4c90d8d to ae78856 Compare January 19, 2022 19:30

gsmet added the triage/backport? label Jan 19, 2022

Postremus force-pushed the webjar-serve-as-jar branch from ae78856 to ed0e5f5 Compare January 20, 2022 06:07

gsmet reviewed Jan 20, 2022

View reviewed changes

Postremus force-pushed the webjar-serve-as-jar branch from ed0e5f5 to e24dd1c Compare January 22, 2022 20:35

Postremus force-pushed the webjar-serve-as-jar branch from e24dd1c to d6da278 Compare January 22, 2022 21:25

Use FileSystemStaticHandler for all webjars

4bbaa52

Postremus force-pushed the webjar-serve-as-jar branch from d6da278 to 4bbaa52 Compare January 23, 2022 18:50

gsmet approved these changes Jan 24, 2022

View reviewed changes

cescoffier approved these changes Jan 24, 2022

View reviewed changes

gsmet merged commit d464b14 into quarkusio:main Jan 24, 2022

quarkus-bot bot added this to the 2.8 - main milestone Jan 24, 2022

gsmet modified the milestones: 2.8 - main, 2.7.0.Final Jan 24, 2022

gsmet removed the triage/backport? label Jan 24, 2022

Postremus deleted the webjar-serve-as-jar branch January 24, 2022 18:34

gsmet reviewed Jan 27, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use FileSystemStaticHandler for all webjars #23030

Use FileSystemStaticHandler for all webjars #23030

Postremus commented Jan 19, 2022

quarkus-bot bot commented Jan 19, 2022 •

edited

Failing Jobs - Building `ae78856`

Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

gsmet left a comment

gsmet Jan 20, 2022

gsmet Jan 20, 2022

Postremus Jan 22, 2022

gsmet Jan 23, 2022

Postremus Jan 23, 2022

Postremus commented Jan 22, 2022

quarkus-bot bot commented Jan 23, 2022

Postremus commented Jan 24, 2022

gsmet commented Jan 24, 2022

gsmet commented Jan 24, 2022

gsmet Jan 27, 2022

Postremus Jan 27, 2022 •

edited

gsmet Jan 27, 2022

Use FileSystemStaticHandler for all webjars #23030

Use FileSystemStaticHandler for all webjars #23030

Conversation

Postremus commented Jan 19, 2022

quarkus-bot bot commented Jan 19, 2022 • edited

Failing Jobs - Building ae78856

Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/smallrye-reactive-messaging-kafka/deployment

gsmet left a comment

Choose a reason for hiding this comment

gsmet Jan 20, 2022

Choose a reason for hiding this comment

gsmet Jan 20, 2022

Choose a reason for hiding this comment

Postremus Jan 22, 2022

Choose a reason for hiding this comment

gsmet Jan 23, 2022

Choose a reason for hiding this comment

Postremus Jan 23, 2022

Choose a reason for hiding this comment

Postremus commented Jan 22, 2022

quarkus-bot bot commented Jan 23, 2022

Failing Jobs - Building 4bbaa52

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

Postremus commented Jan 24, 2022

gsmet commented Jan 24, 2022

gsmet commented Jan 24, 2022

gsmet Jan 27, 2022

Choose a reason for hiding this comment

Postremus Jan 27, 2022 • edited

Choose a reason for hiding this comment

gsmet Jan 27, 2022

Choose a reason for hiding this comment

quarkus-bot bot commented Jan 19, 2022 •

edited

Failing Jobs - Building `ae78856`

Failing Jobs - Building `4bbaa52`

Postremus Jan 27, 2022 •

edited