Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

exception in eval gets written to stderr #145

Closed
phdoerfler opened this Issue · 9 comments

4 participants

@phdoerfler

Hi!

Trying to eval invalid code not only results in an exception (Good!), but in stacktraces written to stderr (Not that good)

The following java code reproduces the bug:

try {
(new javax.script.ScriptEngineManager).getEngineByExtension("rb").eval("1+");
} catch (Exception e) {
// Do nothing
}

No, I totally would not write such code, but it illustrates the problem quite well. Should not print a single line, but does:

[error] SyntaxError:

@headius
Owner

The problem seems to be this code in org.jruby.embed.jsr223.JRubyEngine. I'm not sure why we need to log the output here, since the exception still propagates (in wrapped form) and obviously folks don't like exceptions logging without their knowledge...

    private ScriptException wrapException(Exception e) {
        if (e.getCause() instanceof Exception) {
            Writer w = container.getErrorWriter();
            if (w instanceof PrintWriter) {
                e.printStackTrace((PrintWriter) w);
            } else {
                try {
                    w.write(e.getMessage());
                } catch (IOException ex) {
                    return new ScriptException(ex);
                }
            }
            return new ScriptException((Exception) e.getCause());
        } else {
            return new ScriptException(e);
        }
    }
@yokolet yokolet was assigned
@headius
Owner

@yokolet Can you explain the printing here?

@headius
Owner

I believe we will remove this. Should happen in the 1.7 timeframe.

@qmx

@headius should we use the same logger stuff?

@headius
Owner

@qmx Yeah, we could, but would that silence the default case? It seems to me we should just remove the logging and propagate the wrapped exception. We're already propagating the wrapper, but even if you handle it the original will still be logged. I don't see a reason for that.

@headius headius closed this issue from a commit
@headius headius Remove all places where we unconditionally log exceptions.
Several locations in the embedding APIs unconditionally log
exceptions that are raised to the error stream configured in the
container. They then proceed to throw either the actual exception
or a wrapper, so the user ends up handling it anyway. This causes
expected exceptions to create unnecessary log noise and causes
unexpected exceptions to log twice as much information if they
are not handled by the user.

I have removed all places where exceptions are unconditionally
logged and then subsequently thrown in some form anyway.

Fixes #145.
5af68df
@headius headius closed this in 5af68df
@yokolet
Collaborator

This might be against to JSR223 spec. I''l check it up and write here whether the change is OK or not.

@phdoerfler

Thank's for fixing the issue. I don't know the JSR223 spec, but when using the default Javascript evaluator getEngineByExtension("js"), malformed code does not log an exception, only throws it. That's what I (and others, obviously) expect. I hope JSR223 agrees with that.

@headius
Owner

@yokolet Let me know if you find anything stating that exceptions should be logged. I did a quick read through the spec and could not find anything to indicate this other than one paragraph:

Each of the operations might result in errors. Operations taking
place in the Java Virtual Machine may throw Exceptions. Each error
must be handled so it can be reported in the output from the script. If
the scripting language has an exception mechanism, Java Exceptions
should be converted to script exceptions and these should be thrown
in the calling script.

I interpret this to mean that exceptions should always be included in some output from a script, such as a raised ScriptException, rather than swallowed. I could not find any other part of the spec that indicated logging of exceptions should always occur.

If you find such a statement, we'll evaluate whether to revert those changes. Thanks for keeping us honest! :)

@yokolet
Collaborator

@headius The change looks OK. I read this issue on iPhone since I still don't have ground line of Internet connection and am busy to put things away after a big moving. :( Looking code on iPhone was hard to understand.

For a clarification, let me add a comment.

As you said, JSR223 doesn't have any logging requirement. The spec says a scripting engine should raise checked exceptions. My implementation of logging might have been good for customized environments, which likes logging to some IO rather than getting exceptions. But, I realized it was JRuby specific implementation and should be removed.

It is important for JSR223 imple to work the same as other languages' implementation. So, the change improves compatibility with others.

As for ScriptingContainer, perhaps, "the same logger stuff"(@qmx's comment) should be applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.