Ensure ThreadContext "pre" happens outside try/finally for "post".

jruby · Sep 17, 2014 · e1334fd · e1334fd
1 parent ed54aab
commit e1334fd
Showing 11 changed files with 66 additions and 55 deletions.
diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
@@ -441,8 +441,9 @@ public IRubyObject evalScriptlet(String script, DynamicScope scope) {
         ThreadContext context = getCurrentContext();
         Node rootNode = parseEval(script, "<script>", scope, 0);
 
+        context.preEvalScriptlet(scope);
+
         try {
-            context.preEvalScriptlet(scope);
             return Interpreter.getInstance().execute(this, rootNode, context.getFrameSelf());
         } catch (JumpException.ReturnJump rj) {
             throw newLocalJumpError(RubyLocalJumpError.Reason.RETURN, (IRubyObject)rj.getValue(), "unexpected return");

diff --git a/core/src/main/java/org/jruby/embed/internal/EmbedEvalUnitImpl.java b/core/src/main/java/org/jruby/embed/internal/EmbedEvalUnitImpl.java
@@ -108,11 +108,11 @@ public IRubyObject run() {
 
         // Keep reference to current context to prevent it being collected.
         ThreadContext threadContext = runtime.getCurrentContext();
+        if (sharing_variables) {
+            vars.inject(scope, 0, null);
+            threadContext.pushScope(scope);
+        }
         try {
-            if (sharing_variables) {
-                vars.inject(scope, 0, null);
-                threadContext.pushScope(scope);
-            }
             IRubyObject ret;
             CompileMode mode = runtime.getInstanceConfig().getCompileMode();
             if (mode == CompileMode.FORCE) {

diff --git a/core/src/main/java/org/jruby/embed/internal/EmbedRubyObjectAdapterImpl.java b/core/src/main/java/org/jruby/embed/internal/EmbedRubyObjectAdapterImpl.java
@@ -295,14 +295,16 @@ private <T> T call(MethodType type, Class<T> returnType, RubyObject rubyReceiver
         if (obj != null && obj instanceof Boolean && ((Boolean) obj) == false) {
             sharing_variables = false;
         }
+
+        if (sharing_variables) {
+            ManyVarsDynamicScope scope;
+            if (unit != null && unit.getScope() != null) scope = unit.getScope();
+            else scope = EmbedRubyRuntimeAdapterImpl.getManyVarsDynamicScope(container, 0);
+            container.getVarMap().inject(scope, 0, rubyReceiver);
+            runtime.getCurrentContext().pushScope(scope);
+        }
+
         try {
-            if (sharing_variables) {
-                ManyVarsDynamicScope scope;
-                if (unit != null && unit.getScope() != null) scope = unit.getScope();
-                else scope = EmbedRubyRuntimeAdapterImpl.getManyVarsDynamicScope(container, 0);
-                container.getVarMap().inject(scope, 0, rubyReceiver);
-                runtime.getCurrentContext().pushScope(scope);
-            }
             IRubyObject result = callEachType(type, rubyReceiver, methodName, block, args);
             if (sharing_variables) {
                 container.getVarMap().retrieve(rubyReceiver);

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/CompiledIRMethod.java
@@ -80,15 +80,16 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
             LOG.info("Executing '" + realName + "'");
         }
 
+        if (!hasExplicitCallProtocol) {
+            // update call stacks (push: frame, class, scope, etc.)
+            RubyModule implementationClass = getImplementationClass();
+            context.preMethodFrameAndScope(implementationClass, name, self, block, scope);
+            // FIXME: does not seem right to use this method's visibility as current!!!
+            // See also PushFrame instruction in org.jruby.ir.targets.JVMVisitor
+            context.setCurrentVisibility(Visibility.PUBLIC);
+        }
+
         try {
-            if (!hasExplicitCallProtocol) {
-                // update call stacks (push: frame, class, scope, etc.)
-                RubyModule implementationClass = getImplementationClass();
-                context.preMethodFrameAndScope(implementationClass, name, self, block, scope);
-                // FIXME: does not seem right to use this method's visibility as current!!!
-                // See also PushFrame instruction in org.jruby.ir.targets.JVMVisitor
-                context.setCurrentVisibility(Visibility.PUBLIC);
-            }
             return (IRubyObject)this.method.invokeWithArguments(context, scope, self, args, block, implementationClass);
         } catch (Throwable t) {
             Helpers.throwException(t);

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRMethod.java
@@ -102,9 +102,9 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz
 
         if (method.hasExplicitCallProtocol()) return Interpreter.INTERPRET_METHOD(context, this, self, name, args, block);
 
-        try {
-            pre(context, self, name, block);
+        pre(context, self, name, block);
 
+        try {
             return Interpreter.INTERPRET_METHOD(context, this, self, name, args, block);
         } finally {
             post(context);

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/JavaMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/JavaMethod.java
@@ -110,71 +110,71 @@ public DynamicMethod dup() {
     protected final void preFrameAndScope(ThreadContext context, IRubyObject self, String name, Block block) {
         context.preMethodFrameAndScope(implementationClass, name, self, block, staticScope);
     }
-    
+
     protected final void preFrameAndDummyScope(ThreadContext context, IRubyObject self, String name, Block block) {
         context.preMethodFrameAndDummyScope(implementationClass, name, self, block, staticScope);
     }
-    
+
     protected final void preFrameOnly(ThreadContext context, IRubyObject self, String name, Block block) {
         context.preMethodFrameOnly(implementationClass, name, self, block);
     }
-    
+
     protected final void preScopeOnly(ThreadContext context) {
         context.preMethodScopeOnly(implementationClass, staticScope);
     }
 
     protected final void preNoFrameDummyScope(ThreadContext context) {
         context.preMethodNoFrameAndDummyScope(implementationClass, staticScope);
     }
-    
+
     protected final void preBacktraceOnly(ThreadContext context, String name) {
         context.preMethodBacktraceOnly(name);
     }
 
     protected final void preBacktraceDummyScope(ThreadContext context, String name) {
         context.preMethodBacktraceDummyScope(implementationClass, name, staticScope);
     }
-    
+
     protected final void preBacktraceAndScope(ThreadContext context, String name) {
         context.preMethodBacktraceAndScope(name, implementationClass, staticScope);
     }
 
     protected final void preNoop() {}
-    
+
     protected final static void postFrameAndScope(ThreadContext context) {
         context.postMethodFrameAndScope();
     }
-    
+
     protected final static void postFrameOnly(ThreadContext context) {
         context.postMethodFrameOnly();
     }
-    
+
     protected final static void postScopeOnly(ThreadContext context) {
         context.postMethodScopeOnly();
     }
 
     protected final static void postNoFrameDummyScope(ThreadContext context) {
         context.postMethodScopeOnly();
     }
-    
+
     protected final static void postBacktraceOnly(ThreadContext context) {
         context.postMethodBacktraceOnly();
     }
 
     protected final static void postBacktraceDummyScope(ThreadContext context) {
         context.postMethodBacktraceDummyScope();
     }
-    
+
     protected final static void postBacktraceAndScope(ThreadContext context) {
         context.postMethodBacktraceAndScope();
     }
 
     protected final static void postNoop(ThreadContext context) {}
-    
+
     protected final void callTrace(ThreadContext context, boolean enabled, String name) {
         if (enabled) context.trace(RubyEvent.C_CALL, name, getImplementationClass());
     }
-    
+
     protected final void returnTrace(ThreadContext context, boolean enabled, String name) {
         if (enabled) context.trace(RubyEvent.C_RETURN, name, getImplementationClass());
     }

diff --git a/core/src/main/java/org/jruby/ir/Compiler.java b/core/src/main/java/org/jruby/ir/Compiler.java
@@ -69,8 +69,8 @@ public IRubyObject __file__(ThreadContext context, IRubyObject self, IRubyObject
 
             @Override
             public IRubyObject load(ThreadContext context, IRubyObject self, boolean wrap) {
+                Helpers.preLoadCommon(context, staticScope, false);
                 try {
-                    Helpers.preLoadCommon(context, staticScope, false);
                     return __file__(context, self, IRubyObject.NULL_ARRAY, Block.NULL_BLOCK);
                 } finally {
                     Helpers.postLoad(context);

diff --git a/core/src/main/java/org/jruby/ir/IRScriptBody.java b/core/src/main/java/org/jruby/ir/IRScriptBody.java
@@ -102,11 +102,12 @@ public IRubyObject interpret(ThreadContext context, IRubyObject self) {
         }
 
         IRubyObject retVal;
-        try {
-            scope.setModule(currModule);
-            context.preMethodScopeOnly(currModule, scope);
-            context.setCurrentVisibility(Visibility.PRIVATE);
 
+        scope.setModule(currModule);
+        context.preMethodScopeOnly(currModule, scope);
+        context.setCurrentVisibility(Visibility.PRIVATE);
+
+        try {
             Interpreter.runBeginEndBlocks(getBeginBlocks(), context, self, scope, null);
             retVal = Interpreter.INTERPRET_ROOT(context, self, this, currModule, name);
             Interpreter.runBeginEndBlocks(getEndBlocks(), context, self, scope, null);

diff --git a/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java b/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
@@ -84,11 +84,12 @@ public static IRubyObject interpretCommonEval(Ruby runtime, String file, int lin
 
         IRubyObject rv = null;
         DynamicScope s = null;
-        try {
-            s = rootNode.getScope();
-            s.setEvalType(evalType);
-            context.pushScope(s);
 
+        s = rootNode.getScope();
+        s.setEvalType(evalType);
+        context.pushScope(s);
+
+        try {
             // Since IR introduces additional local vars, we may need to grow the dynamic scope.
             // To do that, IREvalScript has to tell the dyn-scope how many local vars there are.
             // Since the same static scope (the scope within which the eval string showed up)

diff --git a/core/src/main/java/org/jruby/runtime/CompiledIRBlockBody.java b/core/src/main/java/org/jruby/runtime/CompiledIRBlockBody.java
@@ -35,10 +35,12 @@ protected IRubyObject commonYieldPath(ThreadContext context, IRubyObject[] args,
         if (self == null || this.evalType == EvalType.BINDING_EVAL) {
             self = useBindingSelf(binding);
         }
+
+        DynamicScope prevScope = binding.getDynamicScope();
+        DynamicScope newScope  = sharedScope ? prevScope : DynamicScope.newDynamicScope(getStaticScope(), prevScope);
+        context.pushScope(newScope);
+
         try {
-            DynamicScope prevScope = binding.getDynamicScope();
-            DynamicScope newScope  = sharedScope ? prevScope : DynamicScope.newDynamicScope(getStaticScope(), prevScope);
-            context.pushScope(newScope);
             return (IRubyObject)handle.invokeWithArguments(context, getStaticScope(), self, args, block, binding.getMethod(), type);
         } catch (Throwable t) {
             Helpers.throwException(t);

diff --git a/core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java b/core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java
@@ -33,17 +33,20 @@ protected IRubyObject commonYieldPath(ThreadContext context, IRubyObject[] args,
         if (self == null || this.evalType == EvalType.BINDING_EVAL) {
             self = useBindingSelf(binding);
         }
+
         DynamicScope newScope = null;
+        DynamicScope prevScope = binding.getDynamicScope();
+
+        // SSS FIXME: Maybe, we should allocate a NoVarsScope/DummyScope for for-loop bodies because the static-scope here
+        // probably points to the parent scope? To be verified and fixed if necessary. There is no harm as it is now. It
+        // is just wasteful allocation since the scope is not used at all.
+        newScope  = DynamicScope.newDynamicScope(getStaticScope(), prevScope);
+        // Pass on eval state info to the dynamic scope and clear it on the block-body
+        newScope.setEvalType(this.evalType);
+        this.evalType = EvalType.NONE;
+        context.pushScope(newScope);
+
         try {
-            DynamicScope prevScope = binding.getDynamicScope();
-            // SSS FIXME: Maybe, we should allocate a NoVarsScope/DummyScope for for-loop bodies because the static-scope here
-            // probably points to the parent scope? To be verified and fixed if necessary. There is no harm as it is now. It
-            // is just wasteful allocation since the scope is not used at all.
-            newScope  = DynamicScope.newDynamicScope(getStaticScope(), prevScope);
-            // Pass on eval state info to the dynamic scope and clear it on the block-body
-            newScope.setEvalType(this.evalType);
-            this.evalType = EvalType.NONE;
-            context.pushScope(newScope);
             return Interpreter.INTERPRET_BLOCK(context, self, closure, args, binding.getMethod(), block, type);
         }
         finally {