Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[embed] refactor (JSR223) exception wrapping #6457

Merged
merged 39 commits into from Mar 1, 2021

Conversation

kares
Copy link
Member

@kares kares commented Nov 8, 2020

seems it's been a while since this code was visited, there's some cleanup/refactoring along the line.

the motivation is trying to use javax.script APIs and finding things a bit cumbersome, namely:

  • Ruby raises get double packed into an "embed failed exception" which than is wrapped into a javax.script.ScriptException (when using ScriptEngine#invokeMethod),
    believe this is just ineffective - e.g. Nashorn actually exports it's specific exceptions and doesn't always wrap.
    JRuby, since 9.2, has the Java chain for Ruby exceptions - which is very useful with JSR223 (unless it's burried to causes down the line), JRuby should really consider doing a breaking change here ... as proposed.

    also, any throwable gets wrapped (into a InvokeFailedException) even java.lang.Errors, which feels weird.

  • null returns around JRuby's JSR-223 script engine impl, e.g. it was legit to callMethod("", foo)with an empty name that pretty much doesn't actually invoke a Ruby method - the impl simply returned null (and this was actually tested 馃敨).

  • (silent) exception printout(s) and runtime std-out printing ruby exceptions by in embed mode

CHANGE SUMMARY:

  • javax.script embed APIs will no longer wrap with intermediate InvokeFailedException, RaiseExceptions will simply by wrapped one level into a (checked) javax.script.ScriptException
  • ScriptingContainer continues to use the embed exception wrapping by default, but there's a way to disable wrapping using the added constructor
  • "breaking change": only java.lang.Exception types will be wrapped - never java.lang.Error regardless of whether it's coming from a Ruby raise (no way to detect)

Potential Open Questions:

  • do we want to keep org.jruby.embed.InvokeFailedException deprecated given ScriptingContainer will still wrap into those by default
  • maybe a better ScripingContainer API is needed e.g. factory methods where we always turn off wrapping by default (esp. if we keep InvokeFailedException deprecated)

@kares kares marked this pull request as ready for review December 4, 2020 13:55
@headius
Copy link
Member

headius commented Dec 4, 2020

Pre-review comments...

do we want to keep org.jruby.embed.InvokeFailedException deprecated

Since this is still used by default in ScriptingContainer invokes I think we probably can't deprecate it unless we also deprecate the invoke paths that use it. This leads to the second question...

maybe a better ScripingContainer API is needed

I am less excited about adding to the factory API than I would be about deprecating current invoke/eval methods in favor of new ones. Those eval methods have not been touched in over a decade and there are better forms of invoke and eval we could add as a new API. That would allow us to deprecate the old paths and provide a migration path away from old patterns.

For purposes of this PR, which mainly seeks to reduce the wrapping happening inside the javax.script API, I would say let's not deprecate InvokeFailedException until we can work on making a new ScriptingContainer API that doesn't use it at all.

@kares kares added this to the JRuby 9.3.0.0 milestone Dec 7, 2020
@kares
Copy link
Member Author

kares commented Dec 9, 2020

still find some (untested) cases related to exceptions - there is proper javax.script.ScriptException wrapping around eval
but not if a script got "pre-compiled" using ((Compilable) engine).compile("..."); (e.g. in case of a syntax error the non-wrapped exception was thrown previously), should be fixed and properly tested ...

when pre-parsing (compiling) scripts the parser will assume
potential loval variables to be method calls,

since the eval scope won't provide any local variable names
(compared to directly eval-ing with bindings passed in).

see jrubyGH-5876
* master: (235 commits)
  Arity-split Array#sample
  Handle arity errors better in sample
  Arity split and handle bad kwargs in shuffle
  Spec appears to be intermittently failing
  Adjust specs for unpack/pack 'j and 'J' support
  Implement unpack/pack 'j' aand 'J'
  Restore getrlimit and frozen string tags
  Restore two pack tags mistakenly removed
  Sweep passing library tags
  Sweep passing UnboundMethod tags
  Sweep tags for TracePoint
  Sweep passing Time tags
  Sweep passing Process tags
  Sweep passing ObjectSpace tags
  Sweep passing tags for Thread and friends
  Sweep passing tags for Hash, Marshal
  Sweep passing Kernel tags
  Sweep passing tags for Env, Exception, File, IO
  Sweep passing tags for core numerics
  Sweep passing Comparable tags
  ...
@kares kares changed the title cleanup and flatten embed exception chain review embed (JSR223) exception wrapping Dec 17, 2020
@kares
Copy link
Member Author

kares commented Dec 17, 2020

馃煝 after merge, note that the PR does NOT resolve #5876 but we at least explicitly test existing behaviour with compiled scripts.

@kares kares changed the title review embed (JSR223) exception wrapping [embed] refactor (JSR223) exception wrapping Dec 21, 2020
@headius
Copy link
Member

headius commented Feb 28, 2021

This can be merged any time and I will look into enhancing the 223 exception messages with file and line info once it has landed (#6560).

@kares kares merged commit 552e8a1 into jruby:master Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants