7015589: (spec) BufferedWriter.close leaves stream open if close of underlying Writer fails
A few months back there was a bug report [1] pointing out that it's possible to continue writing to a BufferedWriter after invoking its close method and the close fails. A similar issue arises with BufferedOutputStream and other classes. At issue is whether a stream (or more generally a resource) is considered closed in the event that the close method fails. This isn't something that Closeable is clear on. Joe tells me that this didn't come up in the Coin discussions on try-with-resources either. I think it's safe to say that it would be desirable that close releases the resources for failing cases as otherwise the resources may never be released (esp. with try-with-resources usages). Furthermore, when there are multiple resources involved, or cases like the bug report where one stream wraps another, then it would be desirable to keep the state of the resources in sync. To that end, the changes here propose adding an advisory note to AutoCloseable to advise implementers to release the underlying resources and to mark the resource as closed prior to throwing the exception. For the specific buffering classes discussed at the time, these are changed to follow this advise (but their javadoc isn't changed as there may be other implementations or these classes extended in many ways). In the case of BufferedWriter it is also fixed so that any exception from flushing the underlying writer isn't supplanted by the exception from close. In the case of FilterOutputStream (BufferedOutputStream extends it) then it is fixed so that it doesn't ignore the exception thrown when flushing the underlying stream. This is clearly the right thing to do but it does mean that close can now throw an IOException for a case where it didn't before. For now I don't propose to include a compatibility switch but this may be something that has to revisited later. There are other classes in java.io that could also be cleaned up but I don't propose to do a complete audit at this time. The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/7015589/webrev/ Thanks, Alan. [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005732.htm...
On 08/09/2011 01:43 PM, Alan Bateman wrote:
A few months back there was a bug report [1] pointing out that it's possible to continue writing to a BufferedWriter after invoking its close method and the close fails. A similar issue arises with BufferedOutputStream and other classes. At issue is whether a stream (or more generally a resource) is considered closed in the event that the close method fails. This isn't something that Closeable is clear on. Joe tells me that this didn't come up in the Coin discussions on try-with-resources either.
I think it's safe to say that it would be desirable that close releases the resources for failing cases as otherwise the resources may never be released (esp. with try-with-resources usages). Furthermore, when there are multiple resources involved, or cases like the bug report where one stream wraps another, then it would be desirable to keep the state of the resources in sync. To that end, the changes here propose adding an advisory note to AutoCloseable to advise implementers to release the underlying resources and to mark the resource as closed prior to throwing the exception.
For the specific buffering classes discussed at the time, these are changed to follow this advise (but their javadoc isn't changed as there may be other implementations or these classes extended in many ways). In the case of BufferedWriter it is also fixed so that any exception from flushing the underlying writer isn't supplanted by the exception from close. In the case of FilterOutputStream (BufferedOutputStream extends it) then it is fixed so that it doesn't ignore the exception thrown when flushing the underlying stream. This is clearly the right thing to do but it does mean that close can now throw an IOException for a case where it didn't before. For now I don't propose to include a compatibility switch but this may be something that has to revisited later. There are other classes in java.io that could also be cleaned up but I don't propose to do a complete audit at this time.
The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/7015589/webrev/
Thanks, Alan.
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005732.htm...
Let's close() close :) Patch looks good. Rémi
This change looks good. I don't believe a compatibility switch for the FilterOutputStream is warranted. On Aug 9 2011, at 04:43 , Alan Bateman wrote:
A few months back there was a bug report [1] pointing out that it's possible to continue writing to a BufferedWriter after invoking its close method and the close fails. A similar issue arises with BufferedOutputStream and other classes. At issue is whether a stream (or more generally a resource) is considered closed in the event that the close method fails. This isn't something that Closeable is clear on. Joe tells me that this didn't come up in the Coin discussions on try-with-resources either.
I think it's safe to say that it would be desirable that close releases the resources for failing cases as otherwise the resources may never be released (esp. with try-with-resources usages). Furthermore, when there are multiple resources involved, or cases like the bug report where one stream wraps another, then it would be desirable to keep the state of the resources in sync. To that end, the changes here propose adding an advisory note to AutoCloseable to advise implementers to release the underlying resources and to mark the resource as closed prior to throwing the exception.
For the specific buffering classes discussed at the time, these are changed to follow this advise (but their javadoc isn't changed as there may be other implementations or these classes extended in many ways). In the case of BufferedWriter it is also fixed so that any exception from flushing the underlying writer isn't supplanted by the exception from close. In the case of FilterOutputStream (BufferedOutputStream extends it) then it is fixed so that it doesn't ignore the exception thrown when flushing the underlying stream. This is clearly the right thing to do but it does mean that close can now throw an IOException for a case where it didn't before. For now I don't propose to include a compatibility switch but this may be something that has to revisited later. There are other classes in java.io that could also be cleaned up but I don't propose to do a complete audit at this time.
The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/7015589/webrev/
Thanks, Alan.
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005732.htm...
Hi Alan, Sorry for late reply, I somehow miss this mail. The patch seems perfect. Just a curious question about the fix in BufferedWriter: - try { + try (Writer w = out) { flushBuffer(); } finally { - out.close(); out = null; cb = null; } I see this is a java7 new feature (try-catch-resource), but it use a new variable. I am not very familiar with the new feature yet, so is this better than put out.close() in finally block? And if we'd like to fix java6 as well, we may use finally? Not sure if the complied binary would be the same? 于 2011-8-9 19:43, Alan Bateman 写道:
A few months back there was a bug report [1] pointing out that it's possible to continue writing to a BufferedWriter after invoking its close method and the close fails. A similar issue arises with BufferedOutputStream and other classes. At issue is whether a stream (or more generally a resource) is considered closed in the event that the close method fails. This isn't something that Closeable is clear on. Joe tells me that this didn't come up in the Coin discussions on try-with-resources either.
I think it's safe to say that it would be desirable that close releases the resources for failing cases as otherwise the resources may never be released (esp. with try-with-resources usages). Furthermore, when there are multiple resources involved, or cases like the bug report where one stream wraps another, then it would be desirable to keep the state of the resources in sync. To that end, the changes here propose adding an advisory note to AutoCloseable to advise implementers to release the underlying resources and to mark the resource as closed prior to throwing the exception.
For the specific buffering classes discussed at the time, these are changed to follow this advise (but their javadoc isn't changed as there may be other implementations or these classes extended in many ways). In the case of BufferedWriter it is also fixed so that any exception from flushing the underlying writer isn't supplanted by the exception from close. In the case of FilterOutputStream (BufferedOutputStream extends it) then it is fixed so that it doesn't ignore the exception thrown when flushing the underlying stream. This is clearly the right thing to do but it does mean that close can now throw an IOException for a case where it didn't before. For now I don't propose to include a compatibility switch but this may be something that has to revisited later. There are other classes in java.io that could also be cleaned up but I don't propose to do a complete audit at this time.
The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/7015589/webrev/
Thanks, Alan.
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005732.htm...
-- Best Regards, Jimmy, Jing LV
On 08/17/2011 06:41 PM, Jing LV wrote:
Hi Alan,
Sorry for late reply, I somehow miss this mail. The patch seems perfect. Just a curious question about the fix in BufferedWriter:
- try { + try (Writer w = out) { flushBuffer(); } finally { - out.close(); out = null; cb = null; }
I see this is a java7 new feature (try-catch-resource), but it use a new variable.
Yes, you have to declare a new variable, you can't use an expression.
I am not very familiar with the new feature yet, so is this better than put out.close() in finally block?
see http://download.oracle.com/javase/7/docs/technotes/guides/language/try-with-... the difference is the way the exception that is thrown by out.close() if fushBuffer() also throws an exception is managed (see supressed exception)
And if we'd like to fix java6 as well, we may use finally? Not sure if the complied binary would be the same?
It will not be the same but jdk6 has no suppressed exception. Rémi
Hi Alan, I see the status of 7015589 has changed to "fix delivered", so is this fix in the repository(java8?) already? On 2011/8/9 19:43, Alan Bateman wrote:
A few months back there was a bug report [1] pointing out that it's possible to continue writing to a BufferedWriter after invoking its close method and the close fails. A similar issue arises with BufferedOutputStream and other classes. At issue is whether a stream (or more generally a resource) is considered closed in the event that the close method fails. This isn't something that Closeable is clear on. Joe tells me that this didn't come up in the Coin discussions on try-with-resources either.
I think it's safe to say that it would be desirable that close releases the resources for failing cases as otherwise the resources may never be released (esp. with try-with-resources usages). Furthermore, when there are multiple resources involved, or cases like the bug report where one stream wraps another, then it would be desirable to keep the state of the resources in sync. To that end, the changes here propose adding an advisory note to AutoCloseable to advise implementers to release the underlying resources and to mark the resource as closed prior to throwing the exception.
For the specific buffering classes discussed at the time, these are changed to follow this advise (but their javadoc isn't changed as there may be other implementations or these classes extended in many ways). In the case of BufferedWriter it is also fixed so that any exception from flushing the underlying writer isn't supplanted by the exception from close. In the case of FilterOutputStream (BufferedOutputStream extends it) then it is fixed so that it doesn't ignore the exception thrown when flushing the underlying stream. This is clearly the right thing to do but it does mean that close can now throw an IOException for a case where it didn't before. For now I don't propose to include a compatibility switch but this may be something that has to revisited later. There are other classes in java.io that could also be cleaned up but I don't propose to do a complete audit at this time.
The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/7015589/webrev/
Thanks, Alan.
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005732.htm...
Jing Lv wrote:
Hi Alan,
I see the status of 7015589 has changed to "fix delivered", so is this fix in the repository(java8?) already? Yes, it's been in the jdk8 for several weeks:
I see that CR 7015589 was integrated into JDK8 b04. -Chris. On 09/22/11 09:40 AM, Jing Lv wrote:
Hi Alan,
I see the status of 7015589 has changed to "fix delivered", so is this fix in the repository(java8?) already?
On 2011/8/9 19:43, Alan Bateman wrote:
A few months back there was a bug report [1] pointing out that it's possible to continue writing to a BufferedWriter after invoking its close method and the close fails. A similar issue arises with BufferedOutputStream and other classes. At issue is whether a stream (or more generally a resource) is considered closed in the event that the close method fails. This isn't something that Closeable is clear on. Joe tells me that this didn't come up in the Coin discussions on try-with-resources either.
I think it's safe to say that it would be desirable that close releases the resources for failing cases as otherwise the resources may never be released (esp. with try-with-resources usages). Furthermore, when there are multiple resources involved, or cases like the bug report where one stream wraps another, then it would be desirable to keep the state of the resources in sync. To that end, the changes here propose adding an advisory note to AutoCloseable to advise implementers to release the underlying resources and to mark the resource as closed prior to throwing the exception.
For the specific buffering classes discussed at the time, these are changed to follow this advise (but their javadoc isn't changed as there may be other implementations or these classes extended in many ways). In the case of BufferedWriter it is also fixed so that any exception from flushing the underlying writer isn't supplanted by the exception from close. In the case of FilterOutputStream (BufferedOutputStream extends it) then it is fixed so that it doesn't ignore the exception thrown when flushing the underlying stream. This is clearly the right thing to do but it does mean that close can now throw an IOException for a case where it didn't before. For now I don't propose to include a compatibility switch but this may be something that has to revisited later. There are other classes in java.io that could also be cleaned up but I don't propose to do a complete audit at this time.
The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/7015589/webrev/
Thanks, Alan.
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005732.htm...
participants (5)
-
Alan Bateman
-
Chris Hegarty
-
Jing LV
-
Mike Duigou
-
Rémi Forax