Code Review 7000511: PrintStream, PrintWriter, Formatter leave files open when exception thrown

[Mike Duigou]

On Jan 4 2011, at 07:03 , Chris Hegarty wrote:

> On 23/12/2010 20:29, Mike Duigou wrote:
>> Feedback is on webrev.02 http://cr.openjdk.java.net/~chegar/7000511/webrev.02/webrev/ :
>> 
>> * PrintStream
>> 
>> - .flush(), close(), most println() methods  synchronize on this for their entire implementation. They could just be made synchronized methods.
> 
> I think there are pros and cons of doing this, maybe best left to another CR which could look into this separately?

Just an observation. Yes, definitely separately, if at all. 

> 
>> - The javadoc for append(CharSequence,int,int) for the csq==null condition seems misleading as start and end are considered.
> 
> I'm not sure what the original intent of this was, but from what I can see the javadoc appears to be clear and the implementation looks like it behaves correctly.

Rereading it in a new year I'm not as bothered by it. The wording could better reflect that substitution of the string "null" for csq=null is the only special handling for csq=null.

- "An invocation ..." could perhaps be improved to:

This method is equivalent to calling append(CharSequence) with the subsequence of {@code csq}, or the string "null" if {@code csq} is {@code null}, defined by {@code start} and {@code end}. <pre>...</pre>

- @param csq : "If <tt>csq</tt> is ..." sentence to read: 

A {@code null} value is equivalent to passing the string "null".

I feel more strongly about the later change than the former (my alternative wording doesn't seem much better to me).

> 
>> * Formatter
>> 
>> - The zero params constructor could just call this((Apppendable) null). The advantage is that the StringBuilder is only constructed in one place.
>> 
>> - The Formatter(Locale) constructor could just call this(l, (Apppendable) null). Same reason.
> 
> Actually, I think it is clearer to have the StringBuilder created as is. It makes it more obvious than having to trace through another constructor. But it you feel strongly I can make the changes.

Personal preference only. (It's also easy to follow the trail with Netbeans)

> 
> -Chris.
> 
>> 
>> 
>> On Dec 14 2010, at 07:07 , Chris Hegarty wrote:
>> 
>>> Failing java.io.PrintStream, java.io.PrintWriter, java.util.Scanner, and java.util.Formatter multi-arg constructors that take a java.io.File or String filename (as well as one or more additional args) opens a FileIn/OutputStream to the given File/filename. If one of the other given args causes the constructor to fail ( null or unsupported charset for example ) the FileIn/OutputStream is never closed, and the application does not have a reference to it. You rely on the finalizer to close the stream.
>>> 
>>> This is most serious on Windows because you cannot remove a file if there is an open handle to it.
>>> 
>>> I also cleaned up an existing regression test that fails in samevm mode (partly) because of this. And removed another excluded test from the list since its bug was fixed some time ago.
>>> 
>>> Webrev:
>>>  http://cr.openjdk.java.net/~chegar/7000511/webrev.00/webrev/
>>> 
>>> -Chris.
>>