From b7fd2bc8230a9e4b9b6ea39f427e7e50987fb1f3 Mon Sep 17 00:00:00 2001 From: Georgios Andrianakis Date: Wed, 3 Feb 2021 09:25:38 +0200 Subject: [PATCH 1/3] When a change to startup related code is made disable dev mode instrumentation This is done because such code is meant to be run at startup so when it changes, users expect to see it be rerun --- .../dev/RuntimeUpdatesProcessor.java | 43 ++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java b/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java index cdf1eb6e045f1..6d25df4297ad4 100644 --- a/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java +++ b/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java @@ -32,10 +32,14 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import org.jboss.jandex.AnnotationInstance; +import org.jboss.jandex.AnnotationTarget; import org.jboss.jandex.ClassInfo; +import org.jboss.jandex.DotName; import org.jboss.jandex.Index; import org.jboss.jandex.IndexView; import org.jboss.jandex.Indexer; +import org.jboss.jandex.MethodParameterInfo; import org.jboss.logging.Logger; import io.quarkus.bootstrap.runner.Timing; @@ -45,12 +49,18 @@ import io.quarkus.dev.spi.DevModeType; import io.quarkus.dev.spi.HotReplacementContext; import io.quarkus.dev.spi.HotReplacementSetup; +import io.quarkus.runtime.Startup; +import io.quarkus.runtime.StartupEvent; public class RuntimeUpdatesProcessor implements HotReplacementContext, Closeable { private static final Logger log = Logger.getLogger(RuntimeUpdatesProcessor.class); private static final String CLASS_EXTENSION = ".class"; + private static final DotName STARTUP_NAME = DotName.createSimple(Startup.class.getName()); + private static final DotName STARTUP_EVENT_NAME = DotName.createSimple(StartupEvent.class.getName()); + private static final DotName OBSERVES_NAME = DotName.createSimple("javax.enterprise.event.Observes"); + static volatile RuntimeUpdatesProcessor INSTANCE; private final Path applicationRoot; @@ -209,12 +219,14 @@ public boolean doScan(boolean userInitiated) throws IOException { classTransformers.apply(name, bytes)); } Index current = indexer.complete(); - boolean ok = true; - for (ClassInfo clazz : current.getKnownClasses()) { - ClassInfo old = lastStartIndex.getClassByName(clazz.name()); - if (!ClassComparisonUtil.isSameStructure(clazz, old)) { - ok = false; - break; + boolean ok = containsStartupCode(current); + if (ok) { + for (ClassInfo clazz : current.getKnownClasses()) { + ClassInfo old = lastStartIndex.getClassByName(clazz.name()); + if (!ClassComparisonUtil.isSameStructure(clazz, old)) { + ok = false; + break; + } } } @@ -258,6 +270,25 @@ public boolean doScan(boolean userInitiated) throws IOException { return false; } + private boolean containsStartupCode(Index index) { + if (!index.getAnnotations(STARTUP_NAME).isEmpty()) { + return false; + } + List observesInstances = index.getAnnotations(OBSERVES_NAME); + if (!observesInstances.isEmpty()) { + for (AnnotationInstance observesInstance : observesInstances) { + if (observesInstance.target().kind() == AnnotationTarget.Kind.METHOD_PARAMETER) { + MethodParameterInfo methodParameterInfo = observesInstance.target().asMethodParameter(); + short paramPos = methodParameterInfo.position(); + if (STARTUP_EVENT_NAME.equals(methodParameterInfo.method().parameters().get(paramPos).name())) { + return false; + } + } + } + } + return true; + } + @Override public void addPreScanStep(Runnable runnable) { preScanSteps.add(runnable); From f1a603e7349492f525597597af8bbb87970c5063 Mon Sep 17 00:00:00 2001 From: Georgios Andrianakis Date: Wed, 3 Feb 2021 09:36:47 +0200 Subject: [PATCH 2/3] Add setting to allow quarkus:dev to explicitly disable instrumentation --- .../src/main/java/io/quarkus/maven/DevMojo.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java b/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java index 0978339a70477..e77cbdefe0289 100644 --- a/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java +++ b/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java @@ -271,6 +271,14 @@ public class DevMojo extends AbstractMojo { @Parameter(defaultValue = "${quarkus.enforceBuildGoal}") private boolean enforceBuildGoal = true; + /** + * Whether or not Quarkus should disable it's ability to not do a full restart + * when changes to classes are compatible with JVM instrumentation. + * If this is set to true, Quarkus will always restart on changes and never perform class redefinition. + */ + @Parameter(defaultValue = "${disableInstrumentation}") + private boolean disableInstrumentation = false; + @Component private WorkspaceReader wsReader; @@ -823,7 +831,9 @@ private void addQuarkusDevModeDeps(MavenDevModeLauncher.Builder builder) && appDep.getArtifact().getArtifactId().equals("quarkus-ide-launcher"))) { if (appDep.getArtifact().getGroupId().equals("io.quarkus") && appDep.getArtifact().getArtifactId().equals("quarkus-class-change-agent")) { - builder.jvmArgs("-javaagent:" + appDep.getArtifact().getFile().getAbsolutePath()); + if (!disableInstrumentation) { + builder.jvmArgs("-javaagent:" + appDep.getArtifact().getFile().getAbsolutePath()); + } } else { builder.classpathEntry(appDep.getArtifact().getFile()); } From 6643b08a11bb217c28a2a616ade190d81928d2de Mon Sep 17 00:00:00 2001 From: Stuart Douglas Date: Thu, 4 Feb 2021 13:19:58 +1100 Subject: [PATCH 3/3] Change instrumentation enable/disable to use config Also adds a test --- .../io/quarkus/deployment/dev/DevConfig.java | 18 ++++++++ .../dev/RuntimeUpdatesProcessor.java | 21 +++++++-- .../main/java/io/quarkus/maven/DevMojo.java | 12 +---- .../java/io/quarkus/maven/it/DevMojoIT.java | 46 ++++++++++++++++++- .../src/main/java/org/acme/HelloResource.java | 12 +++++ .../src/main/java/org/acme/HelloService.java | 7 +++ 6 files changed, 99 insertions(+), 17 deletions(-) create mode 100644 core/deployment/src/main/java/io/quarkus/deployment/dev/DevConfig.java diff --git a/core/deployment/src/main/java/io/quarkus/deployment/dev/DevConfig.java b/core/deployment/src/main/java/io/quarkus/deployment/dev/DevConfig.java new file mode 100644 index 0000000000000..f2c978730ccc8 --- /dev/null +++ b/core/deployment/src/main/java/io/quarkus/deployment/dev/DevConfig.java @@ -0,0 +1,18 @@ +package io.quarkus.deployment.dev; + +import io.quarkus.runtime.annotations.ConfigItem; +import io.quarkus.runtime.annotations.ConfigPhase; +import io.quarkus.runtime.annotations.ConfigRoot; + +@ConfigRoot(phase = ConfigPhase.BUILD_TIME) +public class DevConfig { + + /** + * Whether or not Quarkus should disable it's ability to not do a full restart + * when changes to classes are compatible with JVM instrumentation. + * If this is set to true, Quarkus will always restart on changes and never perform class redefinition. + */ + @ConfigItem(defaultValue = "true") + boolean instrumentation; + +} diff --git a/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java b/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java index 6d25df4297ad4..066dccb6017eb 100644 --- a/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java +++ b/core/deployment/src/main/java/io/quarkus/deployment/dev/RuntimeUpdatesProcessor.java @@ -32,6 +32,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import org.eclipse.microprofile.config.ConfigProvider; import org.jboss.jandex.AnnotationInstance; import org.jboss.jandex.AnnotationTarget; import org.jboss.jandex.ClassInfo; @@ -219,7 +220,8 @@ public boolean doScan(boolean userInitiated) throws IOException { classTransformers.apply(name, bytes)); } Index current = indexer.complete(); - boolean ok = containsStartupCode(current); + boolean ok = instrumentationEnabled() + && !containsStartupCode(current); if (ok) { for (ClassInfo clazz : current.getKnownClasses()) { ClassInfo old = lastStartIndex.getClassByName(clazz.name()); @@ -270,9 +272,20 @@ public boolean doScan(boolean userInitiated) throws IOException { return false; } + private Boolean instrumentationEnabled() { + ClassLoader old = Thread.currentThread().getContextClassLoader(); + try { + Thread.currentThread().setContextClassLoader(getClass().getClassLoader()); + return ConfigProvider.getConfig() + .getOptionalValue("quarkus.dev.instrumentation", boolean.class).orElse(true); + } finally { + Thread.currentThread().setContextClassLoader(old); + } + } + private boolean containsStartupCode(Index index) { if (!index.getAnnotations(STARTUP_NAME).isEmpty()) { - return false; + return true; } List observesInstances = index.getAnnotations(OBSERVES_NAME); if (!observesInstances.isEmpty()) { @@ -281,12 +294,12 @@ private boolean containsStartupCode(Index index) { MethodParameterInfo methodParameterInfo = observesInstance.target().asMethodParameter(); short paramPos = methodParameterInfo.position(); if (STARTUP_EVENT_NAME.equals(methodParameterInfo.method().parameters().get(paramPos).name())) { - return false; + return true; } } } } - return true; + return false; } @Override diff --git a/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java b/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java index e77cbdefe0289..0978339a70477 100644 --- a/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java +++ b/devtools/maven/src/main/java/io/quarkus/maven/DevMojo.java @@ -271,14 +271,6 @@ public class DevMojo extends AbstractMojo { @Parameter(defaultValue = "${quarkus.enforceBuildGoal}") private boolean enforceBuildGoal = true; - /** - * Whether or not Quarkus should disable it's ability to not do a full restart - * when changes to classes are compatible with JVM instrumentation. - * If this is set to true, Quarkus will always restart on changes and never perform class redefinition. - */ - @Parameter(defaultValue = "${disableInstrumentation}") - private boolean disableInstrumentation = false; - @Component private WorkspaceReader wsReader; @@ -831,9 +823,7 @@ private void addQuarkusDevModeDeps(MavenDevModeLauncher.Builder builder) && appDep.getArtifact().getArtifactId().equals("quarkus-ide-launcher"))) { if (appDep.getArtifact().getGroupId().equals("io.quarkus") && appDep.getArtifact().getArtifactId().equals("quarkus-class-change-agent")) { - if (!disableInstrumentation) { - builder.jvmArgs("-javaagent:" + appDep.getArtifact().getFile().getAbsolutePath()); - } + builder.jvmArgs("-javaagent:" + appDep.getArtifact().getFile().getAbsolutePath()); } else { builder.classpathEntry(appDep.getArtifact().getFile()); } diff --git a/integration-tests/maven/src/test/java/io/quarkus/maven/it/DevMojoIT.java b/integration-tests/maven/src/test/java/io/quarkus/maven/it/DevMojoIT.java index 26e51b017e58d..de0b2406f5a7b 100644 --- a/integration-tests/maven/src/test/java/io/quarkus/maven/it/DevMojoIT.java +++ b/integration-tests/maven/src/test/java/io/quarkus/maven/it/DevMojoIT.java @@ -38,9 +38,9 @@ /** * @author Clement Escoffier - * + *

* NOTE to anyone diagnosing failures in this test, to run a single method use: - * + *

* mvn install -Dit.test=DevMojoIT#methodName */ @DisableForNative @@ -234,6 +234,48 @@ public void testThatInstrumentationBasedReloadWorks() throws MavenInvocationExce //verify that this was an instrumentation based reload Assertions.assertEquals(firstUuid, DevModeTestUtils.getHttpResponse("/app/uuid")); + + source = new File(testDir, "src/main/java/org/acme/HelloService.java"); + filter(source, Collections.singletonMap("\"Stuart\"", "\"Stuart Douglas\"")); + + // Wait until we get "Stuart Douglas" + await() + .pollDelay(100, TimeUnit.MILLISECONDS) + .atMost(1, TimeUnit.MINUTES) + .until(() -> DevModeTestUtils.getHttpResponse("/app/name").contains("Stuart Douglas")); + + //this bean observes startup event, so it should be different UUID + String secondUUid = DevModeTestUtils.getHttpResponse("/app/uuid"); + Assertions.assertNotEquals(secondUUid, firstUuid); + + //now disable instrumentation based restart, and try again + //change it back to hello + DevModeTestUtils.getHttpResponse("/app/disable"); + source = new File(testDir, "src/main/java/org/acme/HelloResource.java"); + filter(source, Collections.singletonMap("return \"" + uuid + "\";", "return \"hello\";")); + + // Wait until we get "hello" + await() + .pollDelay(100, TimeUnit.MILLISECONDS) + .atMost(1, TimeUnit.MINUTES).until(() -> DevModeTestUtils.getHttpResponse("/app/hello").contains("hello")); + + //verify that this was not instrumentation based reload + Assertions.assertNotEquals(secondUUid, DevModeTestUtils.getHttpResponse("/app/uuid")); + secondUUid = DevModeTestUtils.getHttpResponse("/app/uuid"); + + //now re-enable + //and repeat + DevModeTestUtils.getHttpResponse("/app/enable"); + source = new File(testDir, "src/main/java/org/acme/HelloResource.java"); + filter(source, Collections.singletonMap("return \"hello\";", "return \"" + uuid + "\";")); + + // Wait until we get uuid + await() + .pollDelay(100, TimeUnit.MILLISECONDS) + .atMost(1, TimeUnit.MINUTES).until(() -> DevModeTestUtils.getHttpResponse("/app/hello").contains(uuid)); + + //verify that this was an instrumentation based reload + Assertions.assertEquals(secondUUid, DevModeTestUtils.getHttpResponse("/app/uuid")); } @Test diff --git a/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloResource.java b/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloResource.java index 526931d80d983..8b5962d3b5d4b 100644 --- a/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloResource.java +++ b/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloResource.java @@ -43,4 +43,16 @@ public String name() { public String uuid() { return uuid.toString(); } + + @GET + @Path("disable") + public void disable() { + System.setProperty("quarkus.dev.instrumentation", "false"); + } + + @GET + @Path("enable") + public void enable() { + System.setProperty("quarkus.dev.instrumentation","true"); + } } diff --git a/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloService.java b/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloService.java index beab6ed31e750..bfe7fc496c202 100644 --- a/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloService.java +++ b/integration-tests/maven/src/test/resources/projects/classic-inst/src/main/java/org/acme/HelloService.java @@ -1,10 +1,17 @@ package org.acme; +import io.quarkus.runtime.StartupEvent; + import javax.enterprise.context.ApplicationScoped; +import javax.enterprise.event.Observes; @ApplicationScoped public class HelloService { + public void start(@Observes StartupEvent startupEvent) { + + } + public String name() { return "Stuart"; }