From fd65a66545e5b2d9df171c658d0a141ac9d4bf8e Mon Sep 17 00:00:00 2001 From: Denny Crall Date: Thu, 27 Sep 2018 09:41:32 -0500 Subject: [PATCH 1/4] Fix Controller.shouldBeCheckedForEnhancement method Rename the method Controller.shouldBeCheckedForEnhancement to Controller.enhancementCheckComplete to accurately reflect what the code is doing. --- framework/src/play/mvc/Controller.java | 9 ++++----- framework/test-src/play/mvc/ControllerTest.java | 9 +++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/framework/src/play/mvc/Controller.java b/framework/src/play/mvc/Controller.java index b94a8092f0..9a955b1a41 100644 --- a/framework/src/play/mvc/Controller.java +++ b/framework/src/play/mvc/Controller.java @@ -1234,8 +1234,7 @@ private static void verifyContinuationsEnhancement() { } if (haveSeenFirstApplicationClass) { - if (shouldBeCheckedForEnhancement(className)) { - // we're back into the play framework code... + if (enhancementCheckComplete(className)) { return; // done checking } else { // is this class enhanced? @@ -1254,12 +1253,12 @@ private static void verifyContinuationsEnhancement() { /** - * Checks if the classname is from the jdk, sun or play package and therefore should not be checked for enhancement + * Checks if the classname is from the jdk, sun, or play package. These packages indicate that stack inspection is + * back into framework code and the enhancement check is complete. * @param String className * @return boolean */ - static boolean shouldBeCheckedForEnhancement(String className) - { + static boolean enhancementCheckComplete(String className) { return className.startsWith("jdk.") || className.startsWith("sun.") || className.startsWith("play."); } diff --git a/framework/test-src/play/mvc/ControllerTest.java b/framework/test-src/play/mvc/ControllerTest.java index 8f1900511b..b65877c898 100644 --- a/framework/test-src/play/mvc/ControllerTest.java +++ b/framework/test-src/play/mvc/ControllerTest.java @@ -2,13 +2,14 @@ import org.junit.Test; import static org.junit.Assert.*; -import static play.mvc.Controller.shouldBeCheckedForEnhancement; +import static play.mvc.Controller.enhancementCheckComplete; public class ControllerTest{ @Test public void jdkAndPlayClassesShouldNeverBeenCheckedForEnhancement() { - assertTrue(shouldBeCheckedForEnhancement("sun.blah")); - assertTrue(shouldBeCheckedForEnhancement("play.foo")); - assertFalse(shouldBeCheckedForEnhancement("com.bar")); + assertTrue(enhancementCheckComplete("sun.blah")); + assertTrue(enhancementCheckComplete("jdk.base")); + assertTrue(enhancementCheckComplete("play.foo")); + assertFalse(enhancementCheckComplete("com.bar")); } } \ No newline at end of file From 2c297930bd6669066f448b7a025696a79dc836ba Mon Sep 17 00:00:00 2001 From: Denny Crall Date: Sun, 30 Sep 2018 16:53:29 -0500 Subject: [PATCH 2/4] Improve readability for Controller.verifyContinuationsEnhancement() Modified names to mark the entrance into and out of application code to better highlight when we need to verify a class on the stack needs to be enhanced. Extract method for checking if a class is enhanced to not muddy the structure of iterating over the stack. --- framework/src/play/mvc/Controller.java | 42 +++++++++---------- .../test-src/play/mvc/ControllerTest.java | 10 ++--- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/framework/src/play/mvc/Controller.java b/framework/src/play/mvc/Controller.java index 9a955b1a41..388831bf97 100644 --- a/framework/src/play/mvc/Controller.java +++ b/framework/src/play/mvc/Controller.java @@ -1222,46 +1222,46 @@ private static void verifyContinuationsEnhancement() { try { throw new Exception(); + } catch (Exception e) { - boolean haveSeenFirstApplicationClass = false; + boolean inApplicationCode = false; + for (StackTraceElement ste : e.getStackTrace()) { String className = ste.getClassName(); - if (!haveSeenFirstApplicationClass) { - haveSeenFirstApplicationClass = Play.classes.getApplicationClass(className) != null; - // when haveSeenFirstApplicationClass is set to true, we are - // entering the user application code.. + if (!inApplicationCode) { + // Look for an application class to mark start of application code + inApplicationCode = Play.classes.getApplicationClass(className) != null; } - if (haveSeenFirstApplicationClass) { - if (enhancementCheckComplete(className)) { - return; // done checking - } else { - // is this class enhanced? - boolean enhanced = ContinuationEnhancer.isEnhanced(className); - if (!enhanced) { - throw new ContinuationsException( - "Cannot use await/continuations when not all application classes on the callstack are properly enhanced. The following class is not enhanced: " - + className); - } + if(inApplicationCode) { + if(isFrameworkClass(className)) { + // enhancement check complete + return; } + + assertEnhancedClass(className); } } - } } - /** * Checks if the classname is from the jdk, sun, or play package. These packages indicate that stack inspection is * back into framework code and the enhancement check is complete. - * @param String className - * @return boolean */ - static boolean enhancementCheckComplete(String className) { + static boolean isFrameworkClass(String className) { return className.startsWith("jdk.") || className.startsWith("sun.") || className.startsWith("play."); } + private static void assertEnhancedClass(String className) { + if (!ContinuationEnhancer.isEnhanced(className)) { + throw new ContinuationsException( + "Cannot use await/continuations when not all application classes on the callstack are properly enhanced. " + + "The following class is not enhanced: " + className); + } + } + protected static void await(Future future, F.Action callback) { Request.current().isNew = false; Request.current().args.put(ActionInvoker.F, future); diff --git a/framework/test-src/play/mvc/ControllerTest.java b/framework/test-src/play/mvc/ControllerTest.java index b65877c898..cf90ba0dc4 100644 --- a/framework/test-src/play/mvc/ControllerTest.java +++ b/framework/test-src/play/mvc/ControllerTest.java @@ -2,14 +2,14 @@ import org.junit.Test; import static org.junit.Assert.*; -import static play.mvc.Controller.enhancementCheckComplete; +import static play.mvc.Controller.isFrameworkClass; public class ControllerTest{ @Test public void jdkAndPlayClassesShouldNeverBeenCheckedForEnhancement() { - assertTrue(enhancementCheckComplete("sun.blah")); - assertTrue(enhancementCheckComplete("jdk.base")); - assertTrue(enhancementCheckComplete("play.foo")); - assertFalse(enhancementCheckComplete("com.bar")); + assertTrue(isFrameworkClass("sun.blah")); + assertTrue(isFrameworkClass("jdk.base")); + assertTrue(isFrameworkClass("play.foo")); + assertFalse(isFrameworkClass("com.bar")); } } \ No newline at end of file From 7f809c3dbc35c9ab6fbe1dc39caa5145f37d5b2d Mon Sep 17 00:00:00 2001 From: Denny Crall Date: Mon, 1 Oct 2018 11:26:28 -0500 Subject: [PATCH 3/4] Clarify enhancement verification - Renamed isFrameworkClass() to isFrameworkCode(). The check for framework code includes JDK classes. We see these in the stack as the Play framework is reflectively invoking application code. - Pulled the guard clause that detects that we are no longer examing application code away from the enhancement check to highlight that it is not merely checking if the current class should be enhanced but instead signalling the entire verification is complete and that we should exit the method. --- framework/src/play/mvc/Controller.java | 19 ++++++++++--------- .../test-src/play/mvc/ControllerTest.java | 10 +++++----- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/framework/src/play/mvc/Controller.java b/framework/src/play/mvc/Controller.java index 388831bf97..cfa3a7716f 100644 --- a/framework/src/play/mvc/Controller.java +++ b/framework/src/play/mvc/Controller.java @@ -1234,12 +1234,12 @@ private static void verifyContinuationsEnhancement() { inApplicationCode = Play.classes.getApplicationClass(className) != null; } - if(inApplicationCode) { - if(isFrameworkClass(className)) { - // enhancement check complete - return; - } + if(inApplicationCode && isFrameworkCode(className)) { + // When we next see framework code the verification is complete + return; + } + if(inApplicationCode) { assertEnhancedClass(className); } } @@ -1247,11 +1247,12 @@ private static void verifyContinuationsEnhancement() { } /** - * Checks if the classname is from the jdk, sun, or play package. These packages indicate that stack inspection is - * back into framework code and the enhancement check is complete. + * Checks if the classname is from a play package or from the JDK. These packages indicate that stack inspection has + * exited application code and the enhancement check is complete. The JDK classes are encountered through reflective + * handles to Play framework code. */ - static boolean isFrameworkClass(String className) { - return className.startsWith("jdk.") || className.startsWith("sun.") || className.startsWith("play."); + private static boolean isFrameworkCode(String className) { + return className.startsWith("play.") || className.startsWith("jdk.") || className.startsWith("sun."); } private static void assertEnhancedClass(String className) { diff --git a/framework/test-src/play/mvc/ControllerTest.java b/framework/test-src/play/mvc/ControllerTest.java index cf90ba0dc4..9674583a46 100644 --- a/framework/test-src/play/mvc/ControllerTest.java +++ b/framework/test-src/play/mvc/ControllerTest.java @@ -2,14 +2,14 @@ import org.junit.Test; import static org.junit.Assert.*; -import static play.mvc.Controller.isFrameworkClass; +import static play.mvc.Controller.isFrameworkCode; public class ControllerTest{ @Test public void jdkAndPlayClassesShouldNeverBeenCheckedForEnhancement() { - assertTrue(isFrameworkClass("sun.blah")); - assertTrue(isFrameworkClass("jdk.base")); - assertTrue(isFrameworkClass("play.foo")); - assertFalse(isFrameworkClass("com.bar")); + assertTrue(isFrameworkCode("sun.blah")); + assertTrue(isFrameworkCode("jdk.base")); + assertTrue(isFrameworkCode("play.foo")); + assertFalse(isFrameworkCode("com.bar")); } } \ No newline at end of file From 9e57d252b40a1567bb07366a5c00bb66ba4f58bf Mon Sep 17 00:00:00 2001 From: Denny Crall Date: Mon, 1 Oct 2018 11:51:08 -0500 Subject: [PATCH 4/4] Change inApplicationCode to verificationStarted to highlight the boundaries our enhancement checks. --- framework/src/play/mvc/Controller.java | 23 +++++++++---------- .../test-src/play/mvc/ControllerTest.java | 10 ++++---- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/framework/src/play/mvc/Controller.java b/framework/src/play/mvc/Controller.java index cfa3a7716f..0f652d9921 100644 --- a/framework/src/play/mvc/Controller.java +++ b/framework/src/play/mvc/Controller.java @@ -1215,7 +1215,7 @@ protected static T await(Future future) { * leaving actionInvoke into the app, and before reentering Controller.await */ private static void verifyContinuationsEnhancement() { - // only check in dev mode.. + if (Play.mode == Play.Mode.PROD) { return; } @@ -1224,22 +1224,22 @@ private static void verifyContinuationsEnhancement() { throw new Exception(); } catch (Exception e) { - boolean inApplicationCode = false; + boolean verificationStarted = false; for (StackTraceElement ste : e.getStackTrace()) { String className = ste.getClassName(); - if (!inApplicationCode) { - // Look for an application class to mark start of application code - inApplicationCode = Play.classes.getApplicationClass(className) != null; + if (!verificationStarted) { + // Look for first application class to mark start of verification + verificationStarted = Play.classes.getApplicationClass(className) != null; } - if(inApplicationCode && isFrameworkCode(className)) { - // When we next see framework code the verification is complete + if(verificationStarted && shouldNotBeEnhanced(className)) { + // When we next see a class that should not be enhanced, the verification is complete return; } - if(inApplicationCode) { + if(verificationStarted) { assertEnhancedClass(className); } } @@ -1247,11 +1247,10 @@ private static void verifyContinuationsEnhancement() { } /** - * Checks if the classname is from a play package or from the JDK. These packages indicate that stack inspection has - * exited application code and the enhancement check is complete. The JDK classes are encountered through reflective - * handles to Play framework code. + * Checks if the classname is from a play package or from the JDK. These classes should not be enhanced and + * should not be verified. These presence of these packages indicate the verification is complete. */ - private static boolean isFrameworkCode(String className) { + static boolean shouldNotBeEnhanced(String className) { return className.startsWith("play.") || className.startsWith("jdk.") || className.startsWith("sun."); } diff --git a/framework/test-src/play/mvc/ControllerTest.java b/framework/test-src/play/mvc/ControllerTest.java index 9674583a46..297c1458be 100644 --- a/framework/test-src/play/mvc/ControllerTest.java +++ b/framework/test-src/play/mvc/ControllerTest.java @@ -2,14 +2,14 @@ import org.junit.Test; import static org.junit.Assert.*; -import static play.mvc.Controller.isFrameworkCode; +import static play.mvc.Controller.shouldNotBeEnhanced; public class ControllerTest{ @Test public void jdkAndPlayClassesShouldNeverBeenCheckedForEnhancement() { - assertTrue(isFrameworkCode("sun.blah")); - assertTrue(isFrameworkCode("jdk.base")); - assertTrue(isFrameworkCode("play.foo")); - assertFalse(isFrameworkCode("com.bar")); + assertTrue(shouldNotBeEnhanced("sun.blah")); + assertTrue(shouldNotBeEnhanced("jdk.base")); + assertTrue(shouldNotBeEnhanced("play.foo")); + assertFalse(shouldNotBeEnhanced("com.bar")); } } \ No newline at end of file