New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kernel.load(file, true) produces RuntimeException: Should not get here! #3180

Closed
dekellum opened this Issue Jul 24, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@dekellum
Contributor

dekellum commented Jul 24, 2015

Jruby, hasn't had support for the wrap=true option of Kernel::load. While previously (1.6.8, 1.7.0, 1.7.16, 1.7.21) it would not honor this option, it now (9.0.0.0) produces a runtime exception java.lang.RuntimeException: Should not get here! scopeType is SCRIPT_BODY if the wrap option is set.

There was previously a JRUBY-4339 on codehaus:

http://ruby.11.x6.nabble.com/jira-Created-JRUBY-4339-Kernel-load-with-wrap-true-does-not-protect-the-global-namespace-of-calling-m-td3480038.html

The jruby 1.7.0 release notes suggest this was fixed?

http://jruby.org/2012/10/22/jruby-1-7-0.html

...but my testing doesn't agree. Here is a minimal test setup:

https://github.com/dekellum/test-load-wrap

And here is output from MRI (passing); jruby 1.7.0, 1.7.16, 1.7.21 (shows not honoring wrap); and jruby 9.0.0.0 (new RuntimeException raised). For prior 9.x pre-releases I believe #3126 was preventing me from getting this far to see this issue.

% ruby -v loader.rb 
ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-linux]
Top level loader method called from loadee

% jruby -v loader.rb 
jruby 1.7.0 (1.9.3p203) 2012-10-22 ff1ebbe on OpenJDK 64-Bit Server VM 1.8.0_51-b16 +indy [linux-amd64]
Top level loader method called from loadee
RuntimeError: ERROR: Given load() with wrap=true, loadee's foo shouldn't be defined
  (root) at loader.rb:8

% jruby -v loader.rb 
jruby 1.7.16 (1.9.3p392) 2014-09-25 575b395 on OpenJDK 64-Bit Server VM 1.8.0_51-b16 +jit [linux-amd64]
Top level loader method called from loadee
RuntimeError: ERROR: Given load() with wrap=true, loadee's foo shouldn't be defined
  (root) at loader.rb:8

% jruby -v loader.rb 
jruby 1.7.21 (1.9.3p551) 2015-07-07 a741a82 on OpenJDK 64-Bit Server VM 1.8.0_51-b16 +jit [linux-amd64]
Top level loader method called from loadee
RuntimeError: ERROR: Given load() with wrap=true, loadee's foo shouldn't be defined
  (root) at loader.rb:8

% jruby -v loader.rb 
jruby 9.0.0.0 (2.2.2) 2015-07-21 e10ec96 OpenJDK 64-Bit Server VM 25.51-b03 on 1.8.0_51-b16 +jit [linux-amd64]
Top level loader method called from loadee
IRRuntimeHelpers.java:789:in `findInstanceMethodContainer': java.lang.RuntimeException: Should not get here! scopeType is SCRIPT_BODY
    from IRRuntimeHelpers.java:1259:in `defInterpretedInstanceMethod'
    from DefineInstanceMethodInstr.java:56:in `interpret'
    from StartupInterpreterEngine.java:183:in `processOtherOp'
    from StartupInterpreterEngine.java:107:in `interpret'
    from Interpreter.java:116:in `INTERPRET_ROOT'
    from Interpreter.java:103:in `execute'
    from Interpreter.java:32:in `execute'
    from IRTranslator.java:42:in `execute'
    from Ruby.java:837:in `runInterpreter'
    from Ruby.java:2901:in `loadFile'
    from LibrarySearcher.java:245:in `load'
    from LibrarySearcher.java:35:in `load'
    from LoadService.java:335:in `load'
    from RubyKernel.java:966:in `loadCommon'
    from RubyKernel.java:958:in `load19'
    from RubyKernel$INVOKER$s$0$1$load19.gen:-1:in `call'
    from DynamicMethod.java:213:in `call'
    from DynamicMethod.java:209:in `call'
    from CachingCallSite.java:333:in `cacheAndCall'
    from CachingCallSite.java:195:in `call'
    from loader.rb:-1:in `invokeOther7:load'
    from loader.rb:5:in `RUBY$script'
    from MethodHandle.java:625:in `invokeWithArguments'
    from Compiler.java:111:in `load'
    from Ruby.java:821:in `runScript'
    from Ruby.java:813:in `runScript'
    from Ruby.java:751:in `runNormally'
    from Ruby.java:573:in `runFromMain'
    from Main.java:403:in `doRunFromMain'
    from Main.java:298:in `internalRun'
    from Main.java:225:in `run'
    from Main.java:197:in `main'

@enebo enebo added this to the JRuby 1.7.22 milestone Jul 24, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jul 24, 2015

Member

Yeah. Perhaps at some point we regressed on 1.7? At some point we had some large internal changes to LoadService. Crash in 9k is an IR issue but could be directly supported to us not properly supporting wrap.

Member

enebo commented Jul 24, 2015

Yeah. Perhaps at some point we regressed on 1.7? At some point we had some large internal changes to LoadService. Crash in 9k is an IR issue but could be directly supported to us not properly supporting wrap.

@dekellum

This comment has been minimized.

Show comment
Hide comment
@dekellum

dekellum Jul 24, 2015

Contributor

That's what I had imagined, but see above: my test also fails on jruby 1.7.0.

In latest commit test-load-wrap I improved my test a bit. Same results.

Contributor

dekellum commented Jul 24, 2015

That's what I had imagined, but see above: my test also fails on jruby 1.7.0.

In latest commit test-load-wrap I improved my test a bit. Same results.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jul 24, 2015

Member

@dekellum haha...very odd.

Member

enebo commented Jul 24, 2015

@dekellum haha...very odd.

@enebo enebo modified the milestones: JRuby 1.7.22, JRuby 1.7.23 Aug 20, 2015

@enebo enebo modified the milestones: JRuby 1.7.23, JRuby 1.7.24 Nov 24, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

Current results on 1.7 branch:

$ jruby loader.rb
Top level loader method called from loadee
RuntimeError: ERROR: With wrap=true, foo should't be defined at top level
     foo at /Users/headius/projects/jruby-1.7/test-load-wrap/loadee.rb:4
  (root) at loader.rb:8

I'll have a look.

Member

headius commented Jan 15, 2016

Current results on 1.7 branch:

$ jruby loader.rb
Top level loader method called from loadee
RuntimeError: ERROR: With wrap=true, foo should't be defined at top level
     foo at /Users/headius/projects/jruby-1.7/test-load-wrap/loadee.rb:4
  (root) at loader.rb:8

I'll have a look.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

I think I have a patch that fixes wrapping.

Member

headius commented Jan 15, 2016

I think I have a patch that fixes wrapping.

headius added a commit that referenced this issue Jan 15, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

Ok, I've patched this in 1.7 and it seems ok. Both interpreter and AOT scenarios properly isolate definitions when wrap = true.

I'm looking into 9k now, and the error is a bit different:

$ jruby -I. loader.rb 
Top level loader method called from loadee
Unhandled Java exception: java.lang.ClassCastException: org.jruby.RubyObject cannot be cast to org.jruby.RubyModule
java.lang.ClassCastException: org.jruby.RubyObject cannot be cast to org.jruby.RubyModule
   findInstanceMethodContainer at org/jruby/ir/runtime/IRRuntimeHelpers.java:819
  defInterpretedInstanceMethod at org/jruby/ir/runtime/IRRuntimeHelpers.java:1300
                     interpret at org/jruby/ir/instructions/DefineInstanceMethodInstr.java:56
                processOtherOp at org/jruby/ir/interpreter/StartupInterpreterEngine.java:191
                     interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:115
Member

headius commented Jan 15, 2016

Ok, I've patched this in 1.7 and it seems ok. Both interpreter and AOT scenarios properly isolate definitions when wrap = true.

I'm looking into 9k now, and the error is a bit different:

$ jruby -I. loader.rb 
Top level loader method called from loadee
Unhandled Java exception: java.lang.ClassCastException: org.jruby.RubyObject cannot be cast to org.jruby.RubyModule
java.lang.ClassCastException: org.jruby.RubyObject cannot be cast to org.jruby.RubyModule
   findInstanceMethodContainer at org/jruby/ir/runtime/IRRuntimeHelpers.java:819
  defInterpretedInstanceMethod at org/jruby/ir/runtime/IRRuntimeHelpers.java:1300
                     interpret at org/jruby/ir/instructions/DefineInstanceMethodInstr.java:56
                processOtherOp at org/jruby/ir/interpreter/StartupInterpreterEngine.java:191
                     interpret at org/jruby/ir/interpreter/StartupInterpreterEngine.java:115
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

Ok, looks like a hack I put in place while trying to get block call protocol working in JIT was left in, and causes the above error. Removing that hack gets me back to the "should not get here" error. I'll investigate from there.

Member

headius commented Jan 15, 2016

Ok, looks like a hack I put in place while trying to get block call protocol working in JIT was left in, and causes the above error. Removing that hack gets me back to the "should not get here" error. I'll investigate from there.

headius added a commit that referenced this issue Jan 15, 2016

Also get self's class for method defs in script body.
This is part of work for #3180.

This appears to fix issues with load(..., true) not actually
wrapping method definitions in an anonymous module.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

Ok, I think this is working properly on master too now. There were specs on master we did not pass that pass now, so I'm going to call this fixed and close it.

@dekellum If you get a chance, verify jruby-1_7 branch and master builds (or grab tomorrow from http://ci.jruby.org) and let us know that things are fixed for you!

Member

headius commented Jan 15, 2016

Ok, I think this is working properly on master too now. There were specs on master we did not pass that pass now, so I'm going to call this fixed and close it.

@dekellum If you get a chance, verify jruby-1_7 branch and master builds (or grab tomorrow from http://ci.jruby.org) and let us know that things are fixed for you!

@headius headius closed this Jan 15, 2016

dekellum added a commit to dekellum/syncwrap that referenced this issue Jan 18, 2016

Avoid attempting to wrap load of sync file on JRuby 9.0.[0-4]
Since it fails hard (raises) on these versions.

See jruby/jruby#3180

dekellum added a commit to dekellum/syncwrap that referenced this issue Jan 18, 2016

@dekellum

This comment has been minimized.

Show comment
Hide comment
@dekellum

dekellum Jan 18, 2016

Contributor

I can confirm that the 1.7.24-SNAPSHOT (2016-01-18T05:10:56.000Z) is now working fine. The 9.0.5.0-SNAPSHOT (2016-01-18T05:38:12.000Z) is working for the simple test provided above but is failing in a new way in my more complex application (syncwrap, a method included at top-level is no longer found.) I'll try to expand the above simple test to reproduce this, and report back.

Contributor

dekellum commented Jan 18, 2016

I can confirm that the 1.7.24-SNAPSHOT (2016-01-18T05:10:56.000Z) is now working fine. The 9.0.5.0-SNAPSHOT (2016-01-18T05:38:12.000Z) is working for the simple test provided above but is failing in a new way in my more complex application (syncwrap, a method included at top-level is no longer found.) I'll try to expand the above simple test to reproduce this, and report back.

dekellum added a commit to dekellum/test-load-wrap that referenced this issue Jan 18, 2016

Add test of top-level extended bar from loadee.
This is more analogous to the usage in the Syncwrap project.

cc: jruby/jruby#3180
@dekellum

This comment has been minimized.

Show comment
Hide comment
@dekellum

dekellum Jan 18, 2016

Contributor

I updated the test here: dekellum/test-load-wrap@44d50f9 to include this case that still fails on jruby 9.0.5.0-SNAPSHOT, as shown in the last case below. As I can't reopen this issue, I'll wait for a response before opening any new issue. Thanks.

% ruby -v loader.rb
ruby 2.2.4p230 (2015-12-16 revision 53155) [x86_64-linux]
Top level loader method called from loadee
The load(...,true) wrap option works

% jruby -v ./loader.rb 
jruby 1.7.24-SNAPSHOT (1.9.3p551) 2016-01-18 e7e3815 on OpenJDK 64-Bit Server VM 1.8.0_66-b17 +jit [linux-amd64]
Top level loader method called from loadee
The load(...,true) wrap option works

% jruby -v ./loader.rb 
jruby 9.0.5.0-SNAPSHOT (2.2.3) 2016-01-18 7109a23 OpenJDK 64-Bit Server VM 25.66-b17 on 1.8.0_66-b17 +jit [linux-amd64]
NameError: undefined local variable or method `bar' for main:Object
  <top> at /home/david/src/test-load-wrap/loadee.rb:1
   load at org/jruby/RubyKernel.java:955
  <top> at ./loader.rb:10


Contributor

dekellum commented Jan 18, 2016

I updated the test here: dekellum/test-load-wrap@44d50f9 to include this case that still fails on jruby 9.0.5.0-SNAPSHOT, as shown in the last case below. As I can't reopen this issue, I'll wait for a response before opening any new issue. Thanks.

% ruby -v loader.rb
ruby 2.2.4p230 (2015-12-16 revision 53155) [x86_64-linux]
Top level loader method called from loadee
The load(...,true) wrap option works

% jruby -v ./loader.rb 
jruby 1.7.24-SNAPSHOT (1.9.3p551) 2016-01-18 e7e3815 on OpenJDK 64-Bit Server VM 1.8.0_66-b17 +jit [linux-amd64]
Top level loader method called from loadee
The load(...,true) wrap option works

% jruby -v ./loader.rb 
jruby 9.0.5.0-SNAPSHOT (2.2.3) 2016-01-18 7109a23 OpenJDK 64-Bit Server VM 25.66-b17 on 1.8.0_66-b17 +jit [linux-amd64]
NameError: undefined local variable or method `bar' for main:Object
  <top> at /home/david/src/test-load-wrap/loadee.rb:1
   load at org/jruby/RubyKernel.java:955
  <top> at ./loader.rb:10


kares added a commit that referenced this issue Jan 20, 2016

Merge branch 'jruby-1_7'
* jruby-1_7: (27 commits)
  Update jnr-posix to finally fix 32 bit JVM crasher on windows stat
  pend marshaling behavior - not working for Java non-member classes (GH-3526)
  local (non-member) classes should have correct name just like anonymous classes
  DRY out getClassFromPath usage in UnmarshalStream (all delegating to one method)
  review runtime's getClassFromPath
  hide unmarshalObject methods that use private inner type as a parameter
  Update for newer jnr-posix for windows 32 bit JVM stat workaround
  Remove remaining `should` uses from compiler specs.
  Don't deoptimize while loops just because of a block arg.
  Always assign masgn destructured args in block as block-local.
  Move parser scripts to tool.
  Add case for breakage fixed in b93fbb6.
  Check for empty identifiers and treat them as false.
  Same fix for AOT as for interp for #3180.
  Also use the anonymous wrap class for pushRubyClass, for defs.
  More robust, less breaky fix to omit bad class/package elements.
  Revert "Use methods instead of procs for converters."
  Fixes #3550. Warning "io/console not supported; tty will not be manipulated" occurs again on 1.7.23
  updating to jcodings messes with this obscure char no longer being considered a space.
  Update to latest jcodings
  ...

@headius headius reopened this Feb 14, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2016

Member

Oops, catching up...I guess we did fix this in response to #3609.

Member

headius commented Feb 14, 2016

Oops, catching up...I guess we did fix this in response to #3609.

@headius headius closed this Feb 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment