jclouds-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds] JCLOUDS-127 (#32)
Date Mon, 17 Jun 2013 13:27:12 GMT
>     public void addToClose(Closeable toClose) {
>        methodsToClose.add(toClose);
>     }
>  
>     public void close() throws IOException {
> -      Collections.reverse(methodsToClose);
> -      for (Closeable toClose : methodsToClose) {
> -         toClose.close();
> +      if (state.compareAndSet(State.OPEN, State.CLOSING)) {

IMO this change is OK. I can't come up with a use case where closing the context multiple
times would be needed. The main things to be closed in the context are the executor services,
and once they are shutdown I don't see the point in shutting down them again.
Also, once a context is closed, there are no means to "reopen" it, apart from creating a new
one, so I think this won't break any existing code (unless someone is adding arbitrary stuff
to the Closer once the context is created, which I doubt).

Having said this, would it be a good idea to try/catch each call to `toClose.close()` so even
if a Closebale can't be closed we try to close the others? Closing the context should be the
very last operation when using jclouds. I think it should be good to try to close everything
and just leave a log with the close failures.

Anyway, this PR looks good to me and if you @demobox agree, I think it can be merged once
the `isOpen()` comment is addressed, and we can open the try/catch thing as a separate issue.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723718

Mime
  • Unnamed multipart/alternative (inline, 7-Bit, 0 bytes)
View raw message