with-open correctness
Laurent PETIT
There has been a thread on the clojure google group about a problem
with with-open: method .close() is called without wrapping in another
try-catch block.
This can lead to a situation where an exception is thrown by the
user's code, .close()ing also throws an exception in the final block,
and we have the user code's exception shadowed by the closing
exception.
The following code reimplements with-open with new guarantess:
* if with-open's body throws an exception, it is guaranteed that it
is this exception which will be finally propagated to with-open's
caller (differs from the current behaviour)
* if with-open's body does not throw an exception, but closing one
or more 'streams' throws an exception, then the exception resulting
from the call to close() will be propagated to with-open's caller
(equals the current behaviour)
* if with-open's body does not throw an exception, and closing the
'streams" generate no exception, then the result of executing the body
is returned to the caller (equals the current behaviour)
Does this make sense ?
If so, I'll be happy to create an issue for this in JIRA, along with
the appropriate patch.
Ah, there's also one detail in the following code which was not solved
in the referenced thread : should we try to catch Throwables or only
Exceptions in the following code?
(in-ns 'clojure.core)
(defmacro with-open
"bindings => [name init ...]
Evaluates body in a try expression with names bound to the values
of the inits, and a finally clause that calls (.close name) on each
name in reverse order."
{:added "1.0"}
[bindings & body]
(assert-args with-open
(vector? bindings) "a vector for its binding"
(even? (count bindings)) "an even number of forms in binding vector")
(cond
(= (count bindings) 0) `(do ~@body)
(symbol? (bindings 0)) `(let ~(subvec bindings 0 2)
(let [res# (try
(with-open ~(subvec bindings
2) ~@body)
(catch Throwable t#
(try (. ~(bindings 0)
close) (catch Throwable _#)) ; eat close exception
(throw t#)))] ; and
rethrow body exception
(. ~(bindings 0) close)
res#))
:else (throw (IllegalArgumentException.
"with-open only allows Symbols in bindings"))))
Cheers,
--
Laurent
David Powell
On 26 Jan 2011 13:00, "Laurent PETIT" <lauren...@gmail.com> wrote:
>
> Hello,
>
> There has been a thread on the clojure google group about a problem
> with with-open: method .close() is called without wrapping in another
> try-catch block.
This isn't specifically a problem with with-open, it is more a problem with the semantics of finally that clojure borrows from Java. If we want to keep the semantics of finally as-is, perhaps a new 'always' form could be introduced, as an alternative to finally, where exceptions thrown in the try block don't mask the original exception.
David Powell
Er I mean 'in the finally block'.
--
Dave
Laurent PETIT
The user code, in the body of with-open, has no "finally", etc.
So we can make the contract of with-open whatever fits its needs best.
We just need to correct the docstring of with-open, which I indeed
have failed to do in the above 'patch'.
Here it is again, with the corrected docstring :
(in-ns 'clojure.core)
(defmacro with-open
"bindings => [name init ...]
Evaluates body with names bound to the values of the inits, and in any case:
calls (.close name) on each name in reverse order, and returns the
evaluation of body
or propagate an exception thrown by body."
Rich Hickey
Any proposal in this area should consider the various arguments
starting in the thread here:
http://mail.openjdk.java.net/pipermail/coin-dev/2009-February/000011.html
Thanks,
Rich
Laurent PETIT
Laurent PETIT
Ok, so I've done my homework, and here's the result :-)
In a nutshell, by taking care of not swallowing Throwable but only
Exception, our solution and Josh's one are similar.
From the discussion which has followed Josh's proposal, only one point
remained open for Neal Gafter : he simply doesn't like the solution in
the first place which consists of swallowing exceptions potentially
thrown by close() in favor of an exception thrown by the body. His
fear is that an exception thrown by close() may be more "important"
than the body's exception, and should be the one to be propagated.
Josh answer's was that only Errors should be considered more important
than the body's exception (to the point of shadowing them), and his
(and ours) solution covers this.
A more detailed analysis of the proposal and the following Josh/Neal
duel follows:
Overall, the proposed behaviour is semantically identical to what Josh
suggests in his "Automatic Resource Management" proposal.
As far as sugaring is concerned, Josh suggests "complexifying" the
java "try" statement, where Clojure has already chosen to delegate
this responsibility to the "with-open" function (and other languages
such as C#, Python, etc. have also created a special construct for
this, not merged into the exception catching feature). Merging the two
would probably be too much sugar for the own good of both the users
and the implementors, and I don't like this aspect of Josh's proposal.
Considering the implementation. The above suggestion I did differs
only slightly from what Josh suggest:
* I will have to align the level of "catching" Throwable wrt
Exceptions, to align with Josh's.
* Josh's design leverages a mutable boolean, while the
implementation both Ken and I independently wrote uses a simpler
design removing the need for a boolean.
Josh addresses another aspect in his proposal, which is the
introduction of a new new super Interface (Disposable<?>) whose
purpose would be to unify JDK "resources" that could be handled by his
new mechanism ( java.io.Closeable, java.sql.Connection, Statement and
ResultSet). "with-open" currently "solves" this issue with the JDK by
using reflection -aka duck typing-. Unless used inside a giant loop,
this *may* not be a problem. And overall, this is somewhat an
unrelated problem to the clojure's ml thread's OP concerns wrt a
better resource closing strategy.
=> Would you like to address this problem as well, by making with-open
"open" to arbitrary disposable resources, by introducing a Disposable
protocol with just a "close" function, and already retrofitting in
clojure.core all the above mentioned jdk interfaces ?
Josh also mentions "additional Features", let's look at them quickly:
"Retaining suppressed exceptions": would be a way to embark an
exception thrown by a close() call into an exception thrown by the
body. Would require a change into Throwable, adding an
addSuppressedException(). This is outside our field of possibilities,
since we do not want to wrap the original exception, IMHO.
"Ignoring certain close failures" : I don't know whether this would be
desirable or not. This would be a way for the user of the new
tryResource to declare that, for some "kinds of resources", exceptions
thrown by their "close" methods should be silently ignored.
"Expressions in place of declarations" : would open a can of worms
where people could accidentally continue to use a closed resource.
More an explicit statement about a rejected feature than a proposed
additional feature.
Josh also talks about "design alternatives", let's look at them quickly:
"Modifier in place of block" : would be some kind of new java modifier
keyword on local variables declarations, so that those variables would
have their close() methods automatically called when quitting the
scope of those variables. I think this kind of overriding of variable
declaration is not in the spirit of Clojure !
"Annotation to indicate termination method": just an alternative for
being able to declare, in the resource "interface/class" definition,
which method should be called (thus not imposing the method to be
named "close()"). The use of protocols in java solves this problem
once and for all, AFAIK.
Following the proposal email, Neil Gafter engaged the discussion with
Josh. Here's a summary of the interesting parts, and my take on them:
Neil:
* reports a problem in the proposed Disposable<> java interface,
related to the use of generics => Irrelevant for us.
* highlights a few more issues:
* suggests that it is not general enough => Note that he's talking
about the construct as a whole. He does overlook the fact that other
languages have introduced such features over years (C#, Clojure, etc.)
* suggests that they would not have enough time to lift the
suggestion in their release planning. 13 month or more. Hopefully we
do not have any planning constraints, and certainly we'll be able to
vote and include or reject this in less time :-)
* suggests that using closures, every library vendor will be able to
provide a semantically subtly different way of handling the problem.
Of course in clojure this is the case from the beginning => His
suggestion would not be to fix with-open, but to remove it, and let
people all over the world reinvent their own variations with tiny
semantic differences. Insane, really.
Josh:
* patiently deconstructs Neil's arguments (mostly in phase with my
own remarks above)
Neal:
* feels concerned with the fact that the second exception
potentially "indicating serious program failures" would be swallowed
when the body throws.
Josh:
* highlights the fact that only Exceptions are swallowed in favor of
the body's exception, not Errors
* suggests again the introduction of some kind of
addSuppressedException() to Throwable
Neal:
* is not convinced by the "Errors are not swallowed" argument
and the thread stops there.
Laurent PETIT
wise) design could be consider, if we wish to make with-open more
"configurable" wrt user options concerning semantics (and addressing
the points raised by Neil, and optional feature listed by Josh).
a) Neil would like the user to be able to specify exceptions which are
OK to be swallowed in close() in case of exceptions raised both by the
body and by the close() call. Or, more generically, a function taking
the two raised exceptions, and determining which one to rethrow.
b) Josh would like to offer the user the ability to specify, on a
'resource' basis, when it is OK to totally swallow an exception thrown
on close (think an exception thrown by an InputStream.close, if it's
ok for the user and the body didn't throw ...).
We could keep the good looking with-open:
(with-open [r1 (..) (r2 ...)] ....)
and also allow the user to override its default behaviour wrt a) and b) :
:swallow metadata (false by default) on a resource name would indicate
that any throwable sent from a call to close on it should be
swallowed:
(with-open [^:swallow in (BufferedReader. ...)]
; ...
)
:rethrows metadata would accept a function whose responsibility would
be to choose between the two exceptions
It could take a seq arg with first the body's exception, second the
close exception. So functions first and second would be natural
candidate for simple strategies such as "choose body over close" (by
default) or the opposite.
Interestingly, the user passed fn could also return a newly
constructed exception which could also aggregate the two, if the user
wishes so.
(with-open [^{:rethrows second} out (OutputStream. ...)] ; most
important exception always from call to close()
; critical code
)
(with-open [^{:rethrows #(CoumpoundException. (first %) (second %))}
out (OutputStream. ...)] ; rethrows a new coumpound exception
;....
)
(with-open [^{;rethrows (fn [[b c]] (or (and (some #(instance? c)
[Error FatalCustomFooRuntimeException]) c) () b))} out (OutputStream.
...)] ; throw b unless c is Error or FatalCustomFooRuntimeException
; ....
)
or something like that ?
2011/1/26 Laurent PETIT <lauren...@gmail.com>:
Alex Miller
From: Laurent PETIT <lauren...@gmail.com>
To: cloju...@googlegroups.com
Sent: Wed, January 26, 2011 5:18:56 PM
Subject: Re: with-open correctness
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To post to this group, send email to cloju...@googlegroups.com.
To unsubscribe from this group, send email to clojure-dev+unsub...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.
Laurent PETIT
lazy and thought I had just those links to study). More homework for
me .... :)
2011/1/27 Alex Miller <alexd...@yahoo.com>:
> clojure-dev...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/clojure-dev?hl=en.
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clojure Dev" group.
> To post to this group, send email to cloju...@googlegroups.com.
> To unsubscribe from this group, send email to
> clojure-dev...@googlegroups.com.
Laurent PETIT
> FYI if you didn't happen to run across them, there have been more
> discussions on coin-dev about this subject after the thread below. In
> particular, I believe they later changed the spec based on some information
> from doing the implementation. I haven't followed it closely, but maybe it
> would be worth doing a bit more digging.
> Here's some threads from August for example:
> http://mail.openjdk.java.net/pipermail/coin-dev/2010-August/thread.html#2815
So I took the time to read *all* (okay I've skipped a handful of explicitly off
topic ones) threads concerning Automatic Resource Management in the project-coin
ml. I'm totally exhausted ! :-) But also so happy to know and have embraced
Clojure, when I see the kind of problems they have to solve wrt
retro-compatibility of interfaces, generics, et al !
My questions at the end of this message.
In a nutshell
=============
* "Automatic Resource Management" is planned for inclusion in JDK7.
And it is already implemented in open jdk.
* The last reference to its specs I've found is here, right on the ml:
http://mail.openjdk.java.net/pipermail/coin-dev/2010-July/002796.html
* Work has been done with other workgroups to retrofit a maximum of JDK
classes to the new java.lang.AutoCloseable interface ( JDBC 4.1 :
java.sql.Connection, java.sql.ResultSet, java.sql.Statement ;
java.util.Scanner ; java.beans.XMLEncoder ; java.beans.XMLDecoder ;
java.io.ObjectInput ; java.io.ObjectOutput ; javax.sound.sampled.Line ;
javax.sound.midi.Receiver ; javax.sound.midi.Transmitter ;
javax.sound.midi.MidiDevice ;
java.io.Closeable extend java.lang.AutoCloseable ;
Adding a close method to java.nio.channels.FileLock and having FileLock
implement AutoCloseable ; Adding Closeable as an interface implemented by
javax.imageio.stream.ImageInputStream )
Highlight of some features
==========================
* same "default behaviour" semantics as what was suggested on clojure's ml :
the first thrown exception is the one that will finally be propagated
* but they will also introduce a new "suppressed" field to Throwable to be
able to register suppressed exceptions ( .addSuppressed(Throwable),
.getSuppressed() ), and they take care of being able to use this
"suppressed" field in the stacktraces, etc.
* does not attempt to shadow java.lang.Error => in this case, the Error will
still be the one propagated out the body, even if not the first one having
been thrown
* in the ml, a special case for InterruptedException was also considered, if
thrown during the management of the resource closing
* AutoCloseable defines a "public void close() throws Exception" method
signature, which means that the current with-open impl. will work as-is
with all the new retrofitted types added in JDK7. One question remains: do
we want to anticipate this by adding our own AutoCloseable protocol (and
extending it to the same types as the JDK7 will provide) ? Maybe this can
be saved for another day, but now could be the right time to introduce this.
Recently opened questions on the project-coin ml
================================================
* should the call to .close() be guarded by a check on null, in order to
avoid having NPEs thrown by the generated code ?
* there are strong arguments in favor of both camps. I'm more currently on
the should be guarded camp. Why disallow using with-open like this :
(with-open [i (.getResourceAsStream xx)] ...) where getResourceAsStream
may return null and be checked in body. Currently, with-open would throw
a NPE by attempting to blindly call (.close in).
References
==========
http://blogs.sun.com/darcy/entry/project_coin_arm_api
http://blogs.sun.com/darcy/entry/project_coin_bring_close
http://mail.openjdk.java.net/pipermail/coin-dev/2010-July/002796.html
http://mail.openjdk.java.net/pipermail/coin-dev/2010-June/002777.html
QUESTIONS
=========
a. Should we try to stick 'to the point' with the semantics of the JDK7 ARM
(Automatic Resource Management) ?
b. Focusing on which Throwables thrown in the "close mgt code" we'd like to
'suppress', should we adhere to what JDK7 ARM will be doing
(unconditionnally rethrow Errors, special treatment of
InterruptedException, etc.) ?
c. Would it make sense to support, right now, and provide to users of any JDK
supported by Clojure, the same set of 'automatically closeable resources'
as have been considered for JDK7 (by introducing some
clojure.core.AutoCloseable or clojure.core.io.AutoCloseable protocol and
providing from clojure.core.io the extensions to them) ?
d. Does the suggestion of guarding the call to close with a nil check seem
like an interesting improvement over both current with-open and JDK7 ARM ?
e. Should we just do as JDK7 ARM does, e.g. provide fixed default behaviour
for how "the management of the closing calls" is done, or should we
introduce optional configurable keywords, e.g. at name declaration via the
use of metadata (but other options could be suggested ?) ? (more complex,
maybe just letting users not willing to use with-open's semantics could
just ... not use it ?)
Cheers,
--
Laurent
> clojure-dev...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/clojure-dev?hl=en.
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clojure Dev" group.
> To post to this group, send email to cloju...@googlegroups.com.
> To unsubscribe from this group, send email to
> clojure-dev...@googlegroups.com.
Chouser
that material. Just one quick comment from me:
On Wed, Jan 26, 2011 at 4:07 PM, Laurent PETIT <lauren...@gmail.com> wrote:
> "with-open" currently "solves" this issue with the JDK by
> using reflection -aka duck typing-. Unless used inside a giant loop,
> this *may* not be a problem. And overall, this is somewhat an
> unrelated problem to the clojure's ml thread's OP concerns wrt a
> better resource closing strategy.
I think it's fair to call with-open's current behavior duck typing,
but it doesn't necessarily use reflection. If the type of the object
being bound is known at compile time such as via hinting, the .close
call will be compiled without reflection, regardless of whether it's a
java.io.Closeable/close or any other.
Of course the method does still have to be named "close" (this is
with-"open" after all) and take no args, and so leaves open your next
question:
> => Would you like to address this problem as well, by making with-open
> "open" to arbitrary disposable resources, by introducing a Disposable
> protocol with just a "close" function, and already retrofitting in
> clojure.core all the above mentioned jdk interfaces ?
--Chouser
http://joyofclojure.com/
Laurent PETIT
> An amazing body of work there, Laurant, thanks for sifting through all
> that material. Just one quick comment from me:
>
> On Wed, Jan 26, 2011 at 4:07 PM, Laurent PETIT <lauren...@gmail.com> wrote:
>> "with-open" currently "solves" this issue with the JDK by
>> using reflection -aka duck typing-. Unless used inside a giant loop,
>> this *may* not be a problem. And overall, this is somewhat an
>> unrelated problem to the clojure's ml thread's OP concerns wrt a
>> better resource closing strategy.
>
> I think it's fair to call with-open's current behavior duck typing,
> but it doesn't necessarily use reflection. If the type of the object
> being bound is known at compile time such as via hinting, the .close
> call will be compiled without reflection, regardless of whether it's a
> java.io.Closeable/close or any other.
Indeed.
>
> Of course the method does still have to be named "close" (this is
> with-"open" after all) and take no args, and so leaves open your next
> question:
>> => Would you like to address this problem as well, by making with-open
>> "open" to arbitrary disposable resources, by introducing a Disposable
>> protocol with just a "close" function, and already retrofitting in
>> clojure.core all the above mentioned jdk interfaces ?
Or rather, since I've "upgraded" to the final version of the specs
that made it in open jdk for JDK7: not Disposable anymore, but
AutoCloseable.
Mike Hinchey
There is an old issue for something similar I did a long time ago that would be obsolete with what you've proposed: http://dev.clojure.org/jira/browse/CLJ-190
+1 for a protocol and including extensions rather than duck-typing.
+1 for guarding against nil.
David Powell
with-open, to me seems a trivial use of try/finally. The problem being that try/finally potentially swallows the primary exception, making issues harder to diagnose, as that exception was probably more relevant to the diagnosis. I think we should fix that problem by providing an improved version of the finally clause, called, lets say 'always'.
We are in a position to do this as we can add syntax more easily and we can use our syntax to implement resource handling, which Java cannot.
Possible semantics:
If two exceptions are thrown:
a) Discard the secondary one.
b) Add the second via addSuppressedException where available, or add a wrapper to record both.
c) Call Thread.uncaughtException on the second, in most cases this will just log it to stderr.
d) Throw the secondary one, after having called initCause(primary) on the root cause of the secondary.
Making things configurable seems a bit too complicated.
--
Dave
Laurent PETIT
Bloch himself in the thread I've read yesterday, he consider it was a
mistake for them (apparently he was on the working group) for having
done things as they are wrt finally suppressing the try body's
exception.
Now I was not trying to suggest a 'fix' of try/finally in Clojure.
Just focusing on the semantics of "with-open".
Are you suggesting we should work on the core problem, e.g. on
clojure's try/finally ? (and then indeed it would solve, as a special
case, the current problem with with-open).
The problem with "wrapping" both exceptions with a new one is that the
code trying to handle the body's exception will now have to catch both
the original exception, and also our "wrapper" exception, to see if
there is, in it, the body's exception he's interested in.
Would replacing the finally's exception with the body's exception, if
everoby agrees that it does less harm than the contrary (e.g. more
following the principle of least surprise), be a correct pragmatic
choice ?
Of course, handling the special case in with-open allows us to focus
on a specific generated code for with-open.
Handling the special case for clojure's try/finally is probably a
bigger challenge, test wise, edge case wise, performance wise, etc.
2011/1/28 David Powell <djpo...@djpowell.net>:
David Powell
> clojure's try/finally follows java's try/finally semantics. From Josh
> Bloch himself in the thread I've read yesterday, he consider it was a
> mistake for them (apparently he was on the working group) for having
> done things as they are wrt finally suppressing the try body's
> exception.
I had a quick survey of a few languages with exceptions, and finally
in the ones I found had the same semantics as Java's, notable C#.
http://en.wikipedia.org/wiki/Exception_handling_syntax
A similar situation in C++, a destructor called as the stack unwinds
following an exception, throwing an exception, in C++ apparently
causes the runtime to just panic and terminate the process!
http://www.parashift.com/c++-faq-lite/exceptions.html#faq-17.9
> Now I was not trying to suggest a 'fix' of try/finally in Clojure.
> Just focusing on the semantics of "with-open".
>
> Are you suggesting we should work on the core problem, e.g. on
> clojure's try/finally ? (and then indeed it would solve, as a special
> case, the current problem with with-open).
Yes, I think this approach would be worth investigating, because the
problem with finally doesn't seem to be limited to resource closing.
An improved finally clause could be implemented in the Clojure
compiler (generating the necessary byte code and exception blocks).
finally isn't a reserved word in Clojure outside of the try form, so
another clause should be addable to try without breaking any existing
code.
There are some problems with some of the proposals I suggested, I was
just throwing some ideas out there. I think that just discarding the
secondary exception might be a pragmatic option, with the option of
supporting addSuppressedException in future if it ever gets
implemented.
As you say, it is already available in openjdk7:
--
Dave