Clojure

GC Issue 18: with-open allows multiple bindings but silently closes only the first

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

Reported by Chousuke, Dec 18, 2008
in Clojure rev 1175
------------------------------
(import '(java.io StringReader))
(def stream1 (StringReader. "Test"))
(def stream2 (StringReader. "Test2"))

(with-open [a stream1 b stream2] (println "do-nothing"))

(.ready stream2) ; should throw an exception, but doesn't, since stream2 is
not closed

the reason is simple enough: in with-open, the bindings parameter is passed
to let, but in the finally clause, only a simple (.close ~(first bindings))
is emitted.

I have a patch to make with-open support multiple bindings cleanly, but I
do not have a CA so I can't submit it.

--
Jarkko Oranen




Comment 1 by m...@kotka.de, Dec 18, 2008
Reading the docstring I get the impression, that only one binding was intended at the beginning.
However allowing multiple bindings would a) not be a problem, b) justify the let style binding and
c) have pratical relevance.

I attached a simple patch.
 with-open.diff
887 bytes Download
Comment 2 by Chousuke, Dec 19, 2008
The patch is essentially identical to the solution I came up with, except that I used
let* instead of let for safety because destructuring doesn't make sense for with-open.

I'm going to have to go submit a CA so that next time I have an issue with a solution
I can actually submit the patch myself :)
Comment 3 by richhickey, Dec 19, 2008
That is a good point - destructuring is not supported. I don't think let* is the way
to achieve that - no one should be writing let* directly. The macro could just check
that all bindings are to simple symbols.

Also, the closes should happen in reverse order.

This can be achieved by solving the last, most important problem - you can't just do
this with one let, as each binding could potentially fail:

open a
open b
throw exception opening c

b and a never closed.

i.e., multi-binding with-open must use nested lets/trys, maybe could be defined in
terms of recursive calls to current macro.
Comment 4 by m...@kotka.de, Dec 19, 2008
Argh. I should really think, before writing.

Ok. Here the next idea:
with-open calls itself recursively. It checks that the next binding target (?) is a symbol.
Then it assigns just this one binding to a let and again calls itself wrapped in a try. This
gives in particular the reverse order for closing. And in case something bad happens
during opening ensures that previously opened streams are closed.

I had one problem though: Since we check for the bindings being a vector, one has
to take care, that one passes around the bindings again as a vector. Or one uses an
inner macro, where with-open does the check and then passes on to with-open*,
which also accepts a seq.

I took a different route and simply did a subvec instead of a take or drop. Then
we check for the vector several times, but I think this is the least intrusive version.
I had to move the subvec defn upwards, though.

 with-open.diff
2.3 KB Download
Comment 5 by richhickey, Dec 23, 2008
Patch applied (svn 1182) - thanks!
Status: Fixed

People

  • Assignee:
    Unassigned
    Reporter:
    Anonymous
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: