groovy
  1. groovy
  2. GROOVY-3235

DGM.println(Object) and friends do a Writer.close() if the object is a writer. Should be flush() instead.

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.7, 1.6-rc-1
    • Fix Version/s: 1.6-rc-2, 1.5.8
    • Component/s: None
    • Labels:
      None
    • Testcase included:
      yes
    • Number of attachments :
      0

      Description

      The DefaultGroovyMethods:

      print(Object)
      println(Object)
      print(Object, Object)
      println(Object, Object)

      all will create a PrintWriter and then do a Writer.close() if self is a Writer. That will close the underlying writer, which is definitely the wrong thing.

      The correct behavior is to do a flush().

      It is easy to see that flushing rather than closing is the right thing by looking at what happens if self is not a Writer. In that case System.out.print(ln) is called and System.out is not closed.

        Activity

        Hide
        Jim White added a comment - - edited
        Show
        Jim White added a comment - - edited Fix committed - http://fisheye.codehaus.org/changelog/groovy/?cs=14638 + http://fisheye.codehaus.org/changelog/groovy/?cs=14639 Will merge to 1.5 and 1.6 if no objection.
        Hide
        Alexander Veit added a comment -

        To call flush() is the wrong thing either. It is a side effect the user does not want to have. Think e.g. of socket streams where frequent flushes may decrease network throughput. java.io.PrintWriter has auto-flush turned off by default.

        It seems unneccessary to construct a PrintWriter within the DGM methods. The print(ln) methods could use Writer#write(InvokerHelper.toString(value)) followed by an optional new-line Writer#write(System.getProperty("line.separator")).

        Show
        Alexander Veit added a comment - To call flush() is the wrong thing either. It is a side effect the user does not want to have. Think e.g. of socket streams where frequent flushes may decrease network throughput. java.io.PrintWriter has auto-flush turned off by default. It seems unneccessary to construct a PrintWriter within the DGM methods. The print(ln) methods could use Writer#write(InvokerHelper.toString(value)) followed by an optional new-line Writer#write(System.getProperty("line.separator")).
        Hide
        Jim White added a comment -

        I don't have a problem the flush's effect on the underlying writer - this is a convenience method and the user could easily change their code to improve performance.

        I do agree that just using write is better, but I didn't have more time to spend on this issue last night which is why I just fixed the existing approach (creating a PrintWriter). I want to check the implementation of PrintWriter itself and find the best way to Groovy to cache the line.separator property (which it may do already).

        I also have an enhancement (which will be a separate issue) for the non-Writer case so that it will also look for the Script 'out' property (which will also then work with the JSR-223 script engine writer).

        Show
        Jim White added a comment - I don't have a problem the flush's effect on the underlying writer - this is a convenience method and the user could easily change their code to improve performance. I do agree that just using write is better, but I didn't have more time to spend on this issue last night which is why I just fixed the existing approach (creating a PrintWriter). I want to check the implementation of PrintWriter itself and find the best way to Groovy to cache the line.separator property (which it may do already). I also have an enhancement (which will be a separate issue) for the non-Writer case so that it will also look for the Script 'out' property (which will also then work with the JSR-223 script engine writer).
        Hide
        Alexander Veit added a comment -

        Note that PrintWriter#flush() does nothing but flushing the underlying stream. When it is constructed with a Writer PrintWriter it has no internal buffer. So calling flush() does not improve anything.

        Additionally, when calling Writer methods like Writer#write(...) the user is getting the benefit of receiving an exception if something goes wrong. The wrapping PrintWriter would swallow them what is really bad.

        Show
        Alexander Veit added a comment - Note that PrintWriter#flush() does nothing but flushing the underlying stream. When it is constructed with a Writer PrintWriter it has no internal buffer. So calling flush() does not improve anything. Additionally, when calling Writer methods like Writer#write(...) the user is getting the benefit of receiving an exception if something goes wrong. The wrapping PrintWriter would swallow them what is really bad.
        Hide
        Jim White added a comment -

        Ah, good point about PrintWriter.flush being a no-op wrt the PW when constructed over a Writer.

        But as for no exception on problems with the underlying stream, that is a reason to stick with PrintWriter. The API for print/println is that they do not throw checked exceptions. Of course PrintWriter then has checkError and Writer does not.

        I'll remove the flush.

        Show
        Jim White added a comment - Ah, good point about PrintWriter.flush being a no-op wrt the PW when constructed over a Writer. But as for no exception on problems with the underlying stream, that is a reason to stick with PrintWriter. The API for print/println is that they do not throw checked exceptions. Of course PrintWriter then has checkError and Writer does not. I'll remove the flush.
        Hide
        Alexander Veit added a comment -

        Is it really a problem to throw checked exceptions from DGM#print(ln)? Other DGM methods (e.g. leftShift) do this as well. Groovy doesn't care if there's a throws keyword.

        Using PrintWriter adds a lot of unneccessary overhead (object construction, synchonization, stack frames) and the caller has no chance to get a notification if something went wrong. PrintWriter's (stateful) checkError() mechanism is not available for arbitrary Writers.

        Show
        Alexander Veit added a comment - Is it really a problem to throw checked exceptions from DGM#print(ln)? Other DGM methods (e.g. leftShift) do this as well. Groovy doesn't care if there's a throws keyword. Using PrintWriter adds a lot of unneccessary overhead (object construction, synchonization, stack frames) and the caller has no chance to get a notification if something went wrong. PrintWriter's (stateful) checkError() mechanism is not available for arbitrary Writers.
        Hide
        Jim White added a comment -

        I think the behavior should conform to PrintWriter.print(ln) as much as possible. This fix was prompted by these methods closing the underlying Writer, which is clearly bad (and a sign it probably is very little used - which brings up another interesting idea - automated usage profiling and feedback).

        As for performance, this is a convenience method and I'm more interested in brevity and clarity than a constructor call or two. Anyone who is calling this in a loop can use 'new PrintWriter(w).withWriter' or such as they prefer.

        Show
        Jim White added a comment - I think the behavior should conform to PrintWriter.print(ln) as much as possible. This fix was prompted by these methods closing the underlying Writer, which is clearly bad (and a sign it probably is very little used - which brings up another interesting idea - automated usage profiling and feedback). As for performance, this is a convenience method and I'm more interested in brevity and clarity than a constructor call or two. Anyone who is calling this in a loop can use 'new PrintWriter(w).withWriter' or such as they prefer.
        Hide
        Alexander Veit added a comment -

        OK.

        BTW the following test case should probably run through (it does not in 1.5.6) http://jira.codehaus.org/browse/GROOVY-3237.

        Show
        Alexander Veit added a comment - OK. BTW the following test case should probably run through (it does not in 1.5.6) http://jira.codehaus.org/browse/GROOVY-3237 .
        Hide
        blackdrag blackdrag added a comment -

        is this issue really a must for 1.6?

        Show
        blackdrag blackdrag added a comment - is this issue really a must for 1.6?
        Hide
        Jim White added a comment -

        Certainly. Those methods are seriously broken if printing to something like a file which only gets the first print. Folks maybe haven't noticed this because of the habit of printing to a StringWriter which ignores close() calls.

        Show
        Jim White added a comment - Certainly. Those methods are seriously broken if printing to something like a file which only gets the first print. Folks maybe haven't noticed this because of the habit of printing to a StringWriter which ignores close() calls.
        Hide
        blackdrag blackdrag added a comment -

        so what about the issue? The fix is done... what is missing?

        Show
        blackdrag blackdrag added a comment - so what about the issue? The fix is done... what is missing?
        Hide
        Jim White added a comment -

        Committed to 1.6 (cs14853).

        Show
        Jim White added a comment - Committed to 1.6 (cs14853).
        Hide
        Jim White added a comment -

        Committed to 1.5 (14854).

        Show
        Jim White added a comment - Committed to 1.5 (14854).
        Hide
        Roshan Dawrani added a comment -

        The local build on my windows XP machine is failing because of Groovy3235Bug.testBug(). You may want to check. Below is the error details that show up in the log:

        Caused an ERROR
        Expression: (d == t). Values: d = This is one line.
        This is another.
        All these lines should be written.
        , t = This is one line.
        This is another.
        All these lines should be written.

        java.lang.AssertionError: Expression: (d == t). Values: d = This is one line.
        This is another.
        All these lines should be written.
        , t = This is one line.
        This is another.
        All these lines should be written.

        at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:373)
        at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:658)
        at groovy.bugs.Groovy3235Bug.testBug(Groovy3235Bug.groovy:18)

        Show
        Roshan Dawrani added a comment - The local build on my windows XP machine is failing because of Groovy3235Bug.testBug(). You may want to check. Below is the error details that show up in the log: Caused an ERROR Expression: (d == t). Values: d = This is one line. This is another. All these lines should be written. , t = This is one line. This is another. All these lines should be written. java.lang.AssertionError: Expression: (d == t). Values: d = This is one line. This is another. All these lines should be written. , t = This is one line. This is another. All these lines should be written. at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:373) at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:658) at groovy.bugs.Groovy3235Bug.testBug(Groovy3235Bug.groovy:18)
        Hide
        Roshan Dawrani added a comment -

        I have verified both on 1.5.x and 1.6.x dev branches and both builds are failing for me.

        Show
        Roshan Dawrani added a comment - I have verified both on 1.5.x and 1.6.x dev branches and both builds are failing for me.

          People

          • Assignee:
            Jim White
            Reporter:
            Jim White
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: