Exception in finally block

[Efi Merdler]

Hello,
Assume I have

BufferedWriter out = null;
....

try {
....
} catch (IOException e) {
....
}

finally {
if (out != null)
out.close();
}

However close in finally block throws IOException, How can I handle it
?
I do not want my function to throw the exception, is this the only
solution?

Thanks,
Efi

 

[Damian Dunajski]

Hello,
Assume I have

BufferedWriter out = null;
...

try {
...
} catch (IOException e) {
...
}

finally {
if (out != null)
out.close();
}

However close in finally block throws IOException, How can I handle it
?
I do not want my function to throw the exception, is this the only
solution?

Thanks,
Efi

Hello,
try use try-catch block in finally block

For Example:

try {
...
} catch (Exception e) {
...
} finally {
try {
...
} catch (Exception e) {
...
}
}

Damian Dunajski

 

[Thomas Hawtin]

Hello,
Assume I have

BufferedWriter out = null;
...

try {
...
} catch (IOException e) {
...
}

finally {
if (out != null)
out.close();
}

However close in finally block throws IOException, How can I handle it
?
I do not want my function to throw the exception, is this the only
solution?

I think a better way to organise the code is:

try {
Writer rawOut = ...l;
try {
BufferedWriter out = new BufferedWriter(rawOut);
...
out.flush();
} finally {
rawOut.close();
}
} catch (IOException exc) {
...
}

There are two problems with just closing the BufferedWriter: if the
construction of the BufferedWriter fails you will not be able to close
it and there is a bug in BufferedWriter.close reporting exceptions from
flush (in that it doesn't).

If you are doing it a lot, you may like to use the "execute-around" idiom.

Tom Hawtin

 

[Red Orchid]

try {
Writer rawOut = ...l;
try {
BufferedWriter out = new BufferedWriter(rawOut);
...
out.flush();
} finally {
rawOut.close();
}
} catch (IOException exc) {
...
}

What is your comment about the following codes ?

<code_0>
try {
try {
throw new Exception("E_0"); // Exception_0
}
finally {
int i = 5;
i /= 0; // Exception_1
}
}
catch (Exception e) {
e.printStackTrace();
}
</code_0>

The "code_0" above prints only "Exception_1". That is,
"Exception_0" is not printStackTraced.

Next,
<code_1>
try {
throw new Exception("E_0"); // Exception_0
}
catch (Exception e) {
e.printStackTrace();
}
finally {
try {
int i = 5;
i /= 0; // Exception_1
}
catch (Exception e) {
e.printStackTrace();
}
}
</code_1>

The "code_1" above prints both "Exception_0" and "Exception_1".

Last,
<code_2>
void processXX(...) throws IOException {
...
try {
... stream = .....;
// some IOException(*) occurs.
..
}
finally {
stream.close();
}
}
</code_2>

In "code_2" above, if IOException of "stream.close()" occurs,
"IOException(*)" is discarded.

I think that it is better to isolate IOException of "stream.close()"
from other IOExceptions. For example,

<code_3>
void processXX(...) throws IOException, ... {
...
try {
... stream = .....;
// some IOException(*) occurs.
..
}
finally {
try {
stream.close();
}
catch (IOException e) {
// process or throw new IOCloseException(...);
}
}
}
....
class IOCloseException extends Exception {
....
}
</code_3>

 

[Chris Smith]

What is your comment about the following codes ?

Here's my comment. What you get is fairly simple code that reports a
problem. Once you fix that problem, another problem becomes visible.
This works fine. It may initially seem less than ideal, but upon
further examination, there's not really much of a better option. There
are a few reasons for this.

First, it is impossible for a method to reasonably respond to two
different exceptions to its caller. It can't throw both of them. In
order to handle both exceptions, you would need to do so within the
current method. That implies that you scatter code to report unexpected
exceptions all over your code base, which is very ugly. That's what
your code does, for example; it calles e.printStackTrace to print the
exception to standard error; which is, of course, not what you want to
do 95% of the time. Most of the time, you want to write the exception
to a ServletContext log, or email it to an administrator, or pass it to
log4j; but which one depends on the context where the code is used.

Second, it just doesn't matter, in practice, whether you get both
exceptions or not. That's because the second exception generally
reports exactly the same error condition as the first. (That's really
obvious in your I/O stream example; why would you want two different
stack traces representing the same problem with a file?) Even if they
were different, multiple simultaneous failures is hardly the kind of
thing that's worth introducing extra complexity into your code just to
handle marginally better.

I think that it is better to isolate IOException of "stream.close()"
from other IOExceptions. For example,

I'm really curious here. Do you really think thatg closing the stream
is failing for a different reason than the original I/O operation? Is
there even one plausible scenario where that's true (that close() would
have failed, except that something unrelated has gone wrong?)

 

[Red Orchid]

Here's my comment. What you get is fairly simple code that reports a
[snip]
handle marginally better.

My previous article has a connection with Tom Hawtin's article.
I do not want to deduce your comment on his example from
your comment on my previous article. What is your comment
on Tom Hawtin's example ?

There is my additional view of Tom Hawtin's example. First,
the following code is a part of "BufferedWriter" source.

<code>
public void close() throws IOException {
synchronized (lock) {
if (out == null)
return;
flushBuffer();
out.close();
out = null; // #1.
cb = null; //
}
}
</code>

I think that the author of "BufferedWriter" has intention to
assign null to "out" and "cb" when "close()" is called.

But, the following code discards the intention because
"out.close()" is not called.

<code>
//
// quoted from Tom Hawtin's article.
//
try {
Writer rawOut = ...l;
try {
BufferedWriter out = new BufferedWriter(rawOut);
...
out.flush();
} finally {
rawOut.close();
}
} catch (IOException exc) {
...
}
<code>

I'm really curious here. Do you really think thatg closing the stream
is failing for a different reason than the original I/O operation? Is
there even one plausible scenario where that's true (that close() would
have failed, except that something unrelated has gone wrong?)

I think that IOException of steam should not make a mess in
consistency of performance. Lets consider the following code.

<code_2>
//
// quoted and modified from my previous article.
//
void processXX(T1 o1, T2 o2) throws XXException, IOException {

... stream = ...;
try {
{ code block #1} // The state of o1, o2 are changed;
{ code block #2} // data are IOed to/from stream.
....
}
finally {
stream.close();
}
}

//
// Main routine..
//
try {
...
processXX(o1, o2);
...
}
catch (XXException e) {
// restore o1, o2.
}
catch (IOException e) {
// ..
}
</code_2>


The method "processXX" do not guarantee the consistency
of performance.

After { code block #1}, if XXException occurs and successively
IOException of "stream.close()" occurs, the state of o1, o2 can
not be restored to the previous state. Because the XXException
was discarded by the IOException. ( Consider more than one
XXExceptions.)

Closing the stream will be not failing for a different reason than
the original I/O operation. But, IOException of stream.close()
can swallow up other exceptions (XXExceptions).

I think, the failure of "stream.close()" makes no matter in practice.
It is important that other exceptions can be swallowed up.

Do you think that the following code implies that a developer scatter
code to report unexpected exceptions all over his code base ?

<code>
try {
...
}
catch (...) {
...
}
finally {
try {
stream.close();
}
catch (Exception e) {
...
}
}
</code>

 

[Thomas Hawtin]

What is your comment about the following codes ?
try {
try {
throw new Exception("E_0"); // Exception_0
}
finally {
int i = 5;
i /= 0; // Exception_1
}
}
catch (Exception e) {
e.printStackTrace();
}

My first comment is that printStackTrace is not an acceptable exception
handling problem (it really should have "debug" in its name).

My second comment is that the exception from finally should be at least
as significant as the first exception. There shouldn't be much happening
in the finally block. Now, you've switched to a made up example, but the
original, writing to a stream, is common. In that case, if you can't
even close the file you have real problems - at least as bad as not
being able to write to it.

The "code_1" above prints both "Exception_0" and "Exception_1".

So is that going to be two dialog boxes with cryptic and entirely
unhelpful messages?

Tom Hawtin

 

[Thomas Hawtin]

There is my additional view of Tom Hawtin's example. First,
the following code is a part of "BufferedWriter" source.

public void close() throws IOException {
synchronized (lock) {
if (out == null)
return;
flushBuffer();
out.close();
out = null; // #1.
cb = null; //
}
}

I think that the author of "BufferedWriter" has intention to
assign null to "out" and "cb" when "close()" is called.

Look at the code. If the flush throws an exception, the out is not
closed. In fact you can't close out at all through BufferedWriter if you
it is failing on the flush. For 1.5 and earlier you need to flush the
buffer and call close on the underlying stream. Checking the bug database:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6266377

(Something similar exists, and also fixed in 1.6, for BufferedOutputStream.)

But, the following code discards the intention because
"out.close()" is not called.

Who cares. We flushed the buffer. The BufferedWriter is done. All
BufferedWriter.close does extra is to make sure close closes it from
further method calls (which we are not going to do).

The method "processXX" do not guarantee the consistency
of performance.

Then it's badly designed. True exception safety comes through accepting
failure may occur at any point. So it's useless trying to write code for
each failure. Keep it simple.

Tom Hawtin

 

[Red Orchid]

[snip]
My second comment is that the exception from finally should be at least
as significant as the first exception. There shouldn't be much happening
in the finally block. Now, you've switched to a made up example, but the
original, writing to a stream, is common. In that case, if you can't
even close the file you have real problems - at least as bad as not
being able to write to it.

The point of my previous article is that an exception of "finally"
block can discard other exceptions that have to be treated.


This is a part of OP's code. Note that #1 was not defined.
<quote>
try {
.... // code_block: #1
} catch (IOException e) {
....
}
finally {
if (out != null)
out.close();
}
</quote>


This is a part of your previous article.
<quote>
try {
Writer rawOut = ...l;
try {
BufferedWriter out = new BufferedWriter(rawOut);
... // code_block: #2
out.flush();
} finally {
rawOut.close();
}
} catch (IOException exc) {
... // code_block: #3
}
</quote>

In #2, it is practicable for an instance(*) of some class to throw
a sub-class exceptions(*) of IOException.
If "rawOut.close()" throws "IOException", the exceptions(*) can
not be treated in #3 although they have to be treated, because
they were swallowed up by the IOException of "rawOut.close()".

So is that going to be two dialog boxes with cryptic and entirely
unhelpful messages?

Treating the exceptions(*) includes the action of restoring
the instance(*) to the previous state, besides dialog box.

That is,
<code>
Writer rawOut = ...;
try {
BufferedWriter out = new BufferedWriter(rawOut);
...
{code block #4} // XxxxIOException can be thrown.
...
out.close();
}
catch (XxxxIOException e) {
// Restore a related instance of some class to the previous
// state. Or do something that is required.
}
catch (IOException e) {
// dialog box or do something.
}
finally {
try {
rawOut.close();
}
catch (IOException e) {
// process
}
}
</code>


Of course, in #3 of your code, you can roll all the related
instances back. But, to do such policy is to hide problems
that happened. If the policy is valid, why do Java have so
many kinds of exceptions ?

 

[Chris Smith]

My previous article has a connection with Tom Hawtin's article.

Yes, I know. More generally, though, Thomas Hawtin suggested nesting a
try/finally inside of a try/catch. That's a very wise suggestion, and
one that I agree with. You objected, and I answered your objections.
If you want to have a private conversation with Thomas, then email is a
good medium for that.

I do not want to deduce your comment on his example from
your comment on my previous article. What is your comment
on Tom Hawtin's example ?

I think the strategy is sound.

I think that the author of "BufferedWriter" has intention to
assign null to "out" and "cb" when "close()" is called.

Why would it matter? The instance of BufferedWriter is already gone by
the time it might make any difference. The spec for BufferedWriter is
sufficient to guarantee that calling flush() and then discarding the
instance is a perfectly sane thing to do. That's all rather incidental,
though, to the point that try/finally should be separated from try/catch
blocks.

try {
...
processXX(o1, o2);
...
}
catch (XXException e) {
// restore o1, o2.
}
catch (IOException e) {
// ..
}

Yes, that's a problem? What is your alternative? Do you intend to
throw both XXException and IOException from processXX? The language
doesn't allow that. Do you intend to swallow the IOException and just
throw XXException? That's really no better.

My alternative is that if XXException is supposed to be a recoverable
exception, then it should leave the state of o1 and o2 as they were
before throwing XXException. Preferably, this is done by modifying the
logic to not modify them in the first place. If necessary, though, it
can be done by adding another try/catch block in processXX, restoring o1
and o2, and then rethrowing the exception. (Another solution, probably
even better when it is reasonably in the first place, is to eliminate
side-effects from processXX in the first place.)

Do you think that the following code implies that a developer scatter
code to report unexpected exceptions all over his code base ?

finally {
try {
stream.close();
}
catch (Exception e) {
...
}
}

Yes, I do. Otherwise, you would have specified what goes in the catch
block instead of using an ellipse. The problem is that you don't know
what ought to go in the catch block. It depends on how the API is being
used.

 

[Thomas Hawtin]

[snip]
My second comment is that the exception from finally should be at least
as significant as the first exception. There shouldn't be much happening
in the finally block. Now, you've switched to a made up example, but the
original, writing to a stream, is common. In that case, if you can't
even close the file you have real problems - at least as bad as not
being able to write to it.

The point of my previous article is that an exception of "finally"
block can discard other exceptions that have to be treated.

As I say, in any sane system, those other exceptions have become
obsoleted by the finally exception.

In #2, it is practicable for an instance(*) of some class to throw
a sub-class exceptions(*) of IOException.
If "rawOut.close()" throws "IOException", the exceptions(*) can
not be treated in #3 although they have to be treated, because
they were swallowed up by the IOException of "rawOut.close()".

Yup, and it's still obsolete.

Treating the exceptions(*) includes the action of restoring
the instance(*) to the previous state, besides dialog box.

Such clean-up actions are required no matter what the exception. So,
they should be in a finally clause, not in a catch clause. It's
generally much easier to work on a copy rather than attempting some roll
back procedure, so such restoring actions are unnecessary.

catch (XxxxIOException e) {
// Restore a related instance of some class to the previous
// state. Or do something that is required.
}

You need this whatever the exception. i.e. it must be in a finally (or
potentially catch Throwable) block (possibly with a boolean to check
whether try block executed successfully).

catch (IOException e) {
// dialog box or do something.
}
finally {
try {
rawOut.close();
}
catch (IOException e) {
// process

Should be the same problem as the previous, or more urgent.

Of course, in #3 of your code, you can roll all the related
instances back. But, to do such policy is to hide problems
that happened. If the policy is valid, why do Java have so
many kinds of exceptions ?

There are different exceptions so that you can diagnose the problem
(possibly). You are going to have fun trying to disentangle multiple
exceptions at once. The most important exception should be the outermost
one.

Tom Hawtin

 

[Red Orchid]

There are different exceptions so that you can diagnose the problem
(possibly). You are going to have fun trying to disentangle multiple
exceptions at once. The most important exception should be the outermost
one.

I have read your comments.

And,
Do the word "outermost" above mean this ?

<code>
try {
...
}
catch (XException e) {
...
}
catch (YException e) { // <- Outermost
...
}

Or,

try {
try {
...
}
catch (XException e) {
...
}
}
catch (YException e) { // <- Outermost
...
}
</code>


If XException is super class of YException,
YException is unreachable.

 

[Red Orchid]

[snip]

finally {
try {
stream.close();
}
catch (Exception e) {
...
}
}

Yes, I do. Otherwise, you would have specified what goes in the catch
block instead of using an ellipse. The problem is that you don't know
what ought to go in the catch block. It depends on how the API is being
used.

At any rate, it is possible that all the close handling of streams
related with reading/writing data are processed with only one method
in the entire scope of your project. For instance, inside all the "finally"
blocks of your project:
<code>
....
finally {
YourProjectUtility.closeFileStream(stream, file);
}

//
// YourProjectUtility
//
public static void closeFileStream(AnyStreamType stream, File dataFile) {
if (stream != null) {
try {
stream.close();
}
catch (IOException e) {
// Maybe, DialogBox( ..... , dataFile, e.getMessage(), ..);
}
}
}
</code>

 

[Chris Smith]

At any rate, it is possible that all the close handling of streams
related with reading/writing data are processed with only one method
in the entire scope of your project. For instance, inside all the "finally"
blocks of your project:

finally {
YourProjectUtility.closeFileStream(stream, file);
}

That's nice, if all the code that I write is only used within one
application. If, on the other hand, I want to learn from half a century
of software development experience and build modular code that can be
used in a number of different places, then this is a bad idea.

 

[Red Orchid]

That's nice, if all the code that I write is only used within one
application. If, on the other hand, I want to learn from half a century
of software development experience and build modular code that can be
used in a number of different places, then this is a bad idea.

Whether this is a bad idea or not will be depend upon the type of a project.
And, the maintenance related with the above code will not be hard at least
(Including remove and reuse).

 

[Thomas Hawtin]

Do the word "outermost" above mean this ?

By outermost, I mean the one thrown last (from an execution point of
view). There is no retry mechanism in Java (they do not appear to work
well in practice), so exceptions are always thrown outwards.

It's trivial on the acquire side of things, as the code causing the
innermost exception doesn't get to be executed.

try {
...
}
catch (XException e) {
...
}
catch (YException e) { // <- Outermost
...
}

Here both catch blocks would be equally outermost. An exception thrown
from the first catch would not be caught by the second.

try {
try {
...
}
catch (XException e) {
...
}
}
catch (YException e) { // <- Outermost
...
}

Here if the inner catch block threw a YException, then the YException
would be outermost.

If XException is super class of YException,
YException is unreachable.

And your code wouldn't compile.

Tom Hawtin