fix reactor#985 Prevent NPE when debug mode is activated via command …

…line This commit fixes an NPE and subsequent initialization error when the debug mode is activated through the `reactor.trace.operatorStacktrace` System property, by correctly reordering the static fields and blocks. Additionally, the debug mode via command line also uses the correct hook key, so that activating via System property then deactivating via Hooks method would work. Finally, this behavior is tested as an additional test task in Gradle, ensuring the property is set and no previous classloading of Hooks has been done by other tests.
liulangf · Dec 18, 2017 · 75a0ab0 · 75a0ab0
1 parent 14d607b
commit 75a0ab0
Show file tree

Hide file tree

Showing 3 changed files with 107 additions and 26 deletions.
diff --git a/build.gradle b/build.gradle
@@ -354,6 +354,15 @@ project('reactor-core') {
 	}
   }
 
+  task testStaticInit(type: Test, group: 'verification') {
+	systemProperty 'reactor.trace.operatorStacktrace', 'true'
+	includes.clear()
+	include '**/*TestStaticInit.*'
+	doFirst {
+	  println "Additional tests from `testStaticInit` ($includes)"
+	}
+  }
+
   sourceSets.test.resources.srcDirs = ["src/test/resources", "src/test/java"]
 
   if (!JavaVersion.current().isJava9Compatible()) {

diff --git a/reactor-core/src/main/java/reactor/core/publisher/Hooks.java b/reactor-core/src/main/java/reactor/core/publisher/Hooks.java
@@ -463,6 +463,36 @@ static Function<Publisher, Publisher> createOrUpdateOpHook(Collection<Function<?
 		return Collections.unmodifiableMap(onOperatorErrorHooks);
 	}
 
+	static final Logger log = Loggers.getLogger(Hooks.class);
+
+	/**
+	 * A key that can be used to store a sequence-specific {@link Hooks#onErrorDropped(Consumer)}
+	 * hook in a {@link Context}, as a {@link Consumer Consumer&lt;Throwable&gt;}.
+	 */
+	static final String KEY_ON_ERROR_DROPPED = "reactor.onErrorDropped.local";
+	/**
+	 * A key that can be used to store a sequence-specific {@link Hooks#onNextDropped(Consumer)}
+	 * hook in a {@link Context}, as a {@link Consumer Consumer&lt;Object&gt;}.
+	 */
+	static final String KEY_ON_NEXT_DROPPED = "reactor.onNextDropped.local";
+	/**
+	 * A key that can be used to store a sequence-specific {@link Hooks#onOperatorError(BiFunction)}
+	 * hook in a {@link Context}, as a {@link BiFunction BiFunction&lt;Throwable, Object, Throwable&gt;}.
+	 */
+	static final String KEY_ON_OPERATOR_ERROR = "reactor.onOperatorError.local";
+	/**
+	 * A key that can be used to store a sequence-specific {@link Hooks#onOperatorError(BiFunction)}
+	 * hook THAT IS ONLY APPLIED TO Operators{@link Operators#onRejectedExecution(Throwable, Context) onRejectedExecution}
+	 * in a {@link Context}, as a {@link BiFunction BiFunction&lt;Throwable, Object, Throwable&gt;}.
+	 */
+	static final String KEY_ON_REJECTED_EXECUTION = "reactor.onRejectedExecution.local";
+
+	/**
+	 * A key used by {@link #onOperatorDebug()} to hook the debug handler, augmenting
+	 * every single operator with an assembly traceback.
+	 */
+	static final String ON_OPERATOR_DEBUG_KEY = "onOperatorDebug";
+
 	static {
 		onEachOperatorHooks = new LinkedHashMap<>(1);
 		onLastOperatorHooks = new LinkedHashMap<>(1);
@@ -473,7 +503,7 @@ static Function<Publisher, Publisher> createOrUpdateOpHook(Collection<Function<?
 						"false"));
 
 		if (globalTrace) {
-			onEachOperator(OnOperatorDebug.instance());
+			onEachOperator(ON_OPERATOR_DEBUG_KEY, OnOperatorDebug.instance());
 		}
 	}
 
@@ -513,29 +543,4 @@ public Publisher<T> apply(Publisher<T> publisher) {
 		}
 	}
 
-	static final Logger log = Loggers.getLogger(Hooks.class);
-
-	/**
-	 * A key that can be used to store a sequence-specific {@link Hooks#onErrorDropped(Consumer)}
-	 * hook in a {@link Context}, as a {@link Consumer Consumer&lt;Throwable&gt;}.
-	 */
-	static final String KEY_ON_ERROR_DROPPED = "reactor.onErrorDropped.local";
-	/**
-	 * A key that can be used to store a sequence-specific {@link Hooks#onNextDropped(Consumer)}
-	 * hook in a {@link Context}, as a {@link Consumer Consumer&lt;Object&gt;}.
-	 */
-	static final String KEY_ON_NEXT_DROPPED = "reactor.onNextDropped.local";
-	/**
-	 * A key that can be used to store a sequence-specific {@link Hooks#onOperatorError(BiFunction)}
-	 * hook in a {@link Context}, as a {@link BiFunction BiFunction&lt;Throwable, Object, Throwable&gt;}.
-	 */
-	static final String KEY_ON_OPERATOR_ERROR = "reactor.onOperatorError.local";
-	/**
-	 * A key that can be used to store a sequence-specific {@link Hooks#onOperatorError(BiFunction)}
-	 * hook THAT IS ONLY APPLIED TO Operators{@link Operators#onRejectedExecution(Throwable, Context) onRejectedExecution}
-	 * in a {@link Context}, as a {@link BiFunction BiFunction&lt;Throwable, Object, Throwable&gt;}.
-	 */
-	static final String KEY_ON_REJECTED_EXECUTION = "reactor.onRejectedExecution.local";
-
-	static final String ON_OPERATOR_DEBUG_KEY = "onOperatorDebug";
 }
diff --git a/reactor-core/src/test/java/reactor/core/publisher/HooksTestStaticInit.java b/reactor-core/src/test/java/reactor/core/publisher/HooksTestStaticInit.java
@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) 2011-2017 Pivotal Software Inc, All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/*
+ * Copyright (c) 2011-2017 Pivotal Software Inc, All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package reactor.core.publisher;
+
+import org.junit.After;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class HooksTestStaticInit {
+
+	//IMPORTANT: this test case depends on System property
+	// `reactor.trace.operatorStacktrace` be set to `true`
+
+	@After
+	public void resetAllHooks() {
+		Hooks.resetOnOperatorError();
+		Hooks.resetOnNextDropped();
+		Hooks.resetOnErrorDropped();
+//		Hooks.resetOnOperatorDebug(); //superseded by resetOnEachOperator
+		Hooks.resetOnEachOperator();
+		Hooks.resetOnLastOperator();
+	}
+
+	@Test
+	public void syspropDebugModeShouldNotFail() {
+		assertThat(System.getProperties())
+				.as("debug mode set via system property")
+				.containsEntry("reactor.trace.operatorStacktrace", "true");
+		assertThat(Hooks.getOnEachOperatorHooks())
+				.as("debug hook activated")
+				.containsKey(Hooks.ON_OPERATOR_DEBUG_KEY);
+		//would throw NPE due to https://github.com/reactor/reactor-core/issues/985
+		Mono.just("hello").subscribe();
+	}
+
+}