BeanShell incorrectly closing System.out

[davidekholm]

When running BeanShell in contexts where System.out has been redirected, which is common in many environments (jAlbum for instance, where System.out is directed to a special console window) then the System.out print stream is incorrectly closed.

Please never close the output stream! Leave it to the user to decide on such environmental issues. The old adage "Don't do me any favors" really holds true on this one. BeanShell 2.0b6 never had this unwanted behavior.

[nickl-]

@davidekholm Can you please give an example.

[davidekholm]

See Interpreter.java:758:

/** Attempt the release of open resources.
 * @throws IOException */
public void close() throws IOException {
    EOF = true;
    if ( null != getErr() ) {
        if ( !getErr().equals(System.err) )
            getErr().close();
        setErr(null);
    }
    if ( null != getOut() ) {
        if ( !getOut().equals(System.out) )
            getOut().close();
        setOut(null);
    }
    if ( null != getIn() ) {
        getIn().close();
        console.setIn(null);
    }

}

This assumption that if System.out has been redirected, then it should be closed is incorrect and wasn't there in earlier incarnations of BeanShell. It's doing users a "favor" they haven't asked for. As I've mapped System.out to a special console window in our software jAlbum, executing Beanshell statements via JSR-223 now closes this console stream incorrectly.

My request is to restore the behavior like it has always been historically and leave it up to the caller to close any streams. This behavior also breaks the responsibility rule: Let the creator of a stream ensure that it's closed, not the library.

[nickl-]

@davidekholm thank you for the explanation.

I will look into this.

Are you able to patch this behaviour in the meantime or are we holding you back on jAlbum?

[davidekholm]

I've commented-out this code so I've got it working, but it was a drag to fight all the unit tests that expected the stream to be closed :-/. I'm fine for now but naturally want to be able to directly use any official builds of BeanShell without further modifications.

[nickl-]

I think it had something to do with resource leaks, we of course do not have to close System.out as java will take care of that but other resources we do.

I assume you are specifically concerned with the behaviour of JSR-223 which makes sense. Since we opened the streams and you do not have access to the interpreter the onus lies on us to close them, which is not the behaviour you are expecting. Will this assumption be correct?

How exactly are you modifying the output stream?

Perhaps a better patch would be made to ScriptEngine instead of commenting out the close functionality, which is needed for when you do have access to the interpreter. I am open to suggestions on how better to deal with this. I don't think it is something which is catered for by JSR-223 or am I mistaken?

[nickl-]

Which tests are failing and what are they testing for exactly?

[davidekholm]

No you didn't open the stream that gets closed by Interpreter. Not when I use BeanShell via JSR-223 anyway. I simply use System.setOut() to direct any System.out.println output to jAlbum's own console window (a Swing based window), but after executing BeanShell statements, System.out is now closed.

If I comment out the code that closes System.out and System.err then I get a host of unit test errors. Don't remember exactly which ones (I commented them out too). Just comment out and see!

[davidekholm]

The unit tests assert that System.out and System.err are indeed closed after running Interpreter.eval if I remember correctly.

[nickl-]

No you didn't open the stream that gets closed by Interpreter.

Perhaps I wasn't clear, within the script engine we manage the interpreter, which is AutoCloseable and therefor needs to be closed by us.

See:

https://github.com/beanshell/beanshell/blob/517392ff86f2045d22956c143797c56aa37929dd/src/main/java/bsh/engine/BshScriptEngine.java#L90-L104

Not when I use BeanShell via JSR-223 anyway. I simply use System.setOut() to direct any System.out.println output to jAlbum's own console window (a Swing based window), but after executing BeanShell statements, System.out is now closed.

That is odd, we should be checking if bsh.out != System.out so if you are changing System.out that would be the new "do not close" normal.

https://github.com/beanshell/beanshell/blob/517392ff86f2045d22956c143797c56aa37929dd/src/main/java/bsh/Interpreter.java#L768-L769

So there should be no reason to close the stream, is there perhaps a problem with the custom streams equals() function which causes it to return false?

If I comment out the code that closes System.out and System.err then I get a host of unit test errors. Don't remember exactly which ones (I commented them out too). Just comment out and see!

Unfortunately I am not at a development environment right now, I made some time to look into outstanding issues and see if I can help you out. The more information you can give me the better I may be able to assist.

[nickl-]

Conclusion: beanshell does not close System.out

https://github.com/beanshell/beanshell/blob/517392ff86f2045d22956c143797c56aa37929dd/src/main/java/bsh/Interpreter.java#L768-L769

Suggestion: see why custom output's equals() function returns false.

Thank you for your patience.

[nickl-]

Todo: add unit test proving System.setOut() and bsh.close() leaves System.out open.

For extra Joyce...

[davidekholm]

Remember I'm not interacting with the Interpreter object directly. I'm using BeanShell via JSR-223 so I'm using BshScriptEngine. In short. I'm not responsible for setting anything in particular. I can only see that I'm initializing BeanShell AFTER calling System.setOut. I guess that is what makes your logic think that System.out hasn't been altered. I hope you can adjust the BeanShell logic so it doesn't require BeanShell to be initiated at a certain point in relation to the System.setOut call. Now it has side effects.

[nickl-]

We are getting our lines crossed and misunderstanding each other.

What I am saying is BeanShell does NOT close System out. See:

https://github.com/beanshell/beanshell/blob/517392ff86f2045d22956c143797c56aa37929dd/src/main/java/bsh/Interpreter.java#L768-L769

If Interpreter's out is equal to system's out then it will not close Interpreter's out.

What we need is a unit test which first calls System.setOut then runs BshScriptEngine and tests that System.out is still open. That is all we need to do because BeanShell does NOT close System out and the test will prove it.

[davidekholm]

Nick, if System.setOut() is called AFTER the interpreter has been initialized, then BeanShell will close System.out

[nickl-]

Ok I hear you:

Interpreter.out = System.out;
// call to System.setOut()
System.out = Custom.out;
// Interpreter onClose handler
Interpreter.out != System.out;
// translates to
OriginalSystem.out != CurrentSystem.out;
// Interpreter closes the OriginalSystem.out

What a pickle!!!! Is there another way to identify the actual System print stream? I don't know?!?!?!

Does it work correctly if you call System.setOut before initialising the interpreter?

[davidekholm]

I don't think there is another way to identify the actual print stream. I assume one can rewrite and call System.setOut first, but REALLY, closing the passed stream is breaking the rule of concerns. You should only close streams CREATED by you, never close streams PASSED to you.

[nickl-]

You are right, my apologies. It's been a long couple of months.

This should be resolved now.