groovy

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

Ascending order - Click to sort in descending order
Hide
Permalink
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.

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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Jim White added a comment -

Committed to 1.6 (cs14853).

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

Committed to 1.5 (14854).

Show
Jim White added a comment - Committed to 1.5 (14854).
Hide
Permalink
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
Permalink
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
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: