You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Zack Shoylev <no...@github.com> on 2016/09/07 20:25:58 UTC
[jclouds/jclouds] Some handles were not properly closed (#1007)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1007
-- Commit Summary --
* Some handles were not properly closed
-- File Changes --
M apis/openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/blobstore/RegionScopedSwiftBlobStore.java (7)
M apis/openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/blobstore/RegionScopedSwiftBlobStoreParallelLiveTest.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1007.patch
https://github.com/jclouds/jclouds/pull/1007.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -701,14 +702,11 @@ public void downloadBlob(String container, String name, File destination, Execut
> Futures.getUnchecked(Futures.allAsList(results));
>
> } catch (IOException e) {
> - // cleanup, attempt to delete large file
> - if (raf != null) {
> - try {
> - raf.close();
> - } catch (IOException e1) {}
> - }
> + Closeables2.closeQuietly(raf);
Remove this and let it be closed in the finally block
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r77900268
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
@zack-shoylev pushed 1 commit.
f795b01 Also replace some System.out calls
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4..f795b01c7149f87337e2560f1f36126670d7bb97
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
merged
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007#issuecomment-245425200
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -819,10 +817,7 @@ public void run() {
> }
> }
> // Finished writing results to stream
> - try {
> - output.close();
> - } catch (IOException e) {
> - }
> + Closeables2.closeQuietly(output);
I've just seen a bunch of "System.out" in this class. I know it is not the scope of this PR, but it would be great to replace them by a logger!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r77900559
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -709,6 +710,12 @@ public void downloadBlob(String container, String name, File destination, Execut
> }
> destination.delete();
> throw new RuntimeException(e);
> + } finally {
> + if (raf != null) {
> + try {
> + raf.close();
> + } catch (IOException e1) {}
Use `Closeables2.closeQuietly` instead?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/ef80208da1149c5d1545d6cbf5c182491545ffa0#r77898630
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
> @@ -819,10 +817,7 @@ public void run() {
> }
> }
> // Finished writing results to stream
> - try {
> - output.close();
> - } catch (IOException e) {
> - }
> + Closeables2.closeQuietly(output);
Good idea
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r77902010
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
> @@ -701,14 +702,11 @@ public void downloadBlob(String container, String name, File destination, Execut
> Futures.getUnchecked(Futures.allAsList(results));
>
> } catch (IOException e) {
> - // cleanup, attempt to delete large file
> - if (raf != null) {
> - try {
> - raf.close();
> - } catch (IOException e1) {}
> - }
> + Closeables2.closeQuietly(raf);
That won't work, it's trying to delete the file for cleanup; it can't do that if it' open. Which kinda sucks.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r77900714
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
@zack-shoylev pushed 1 commit.
0003721 Fixes
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/ef80208da1149c5d1545d6cbf5c182491545ffa0..0003721c3e983e3f3c08824d86cbc7e5098f4eb4
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -700,6 +700,7 @@ public void downloadBlob(String container, String name, File destination, Execut
>
> Futures.getUnchecked(Futures.allAsList(results));
>
> + raf.close();
This is redundant; it will reach the `finally` block immediately.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/ef80208da1149c5d1545d6cbf5c182491545ffa0#r77898298
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -700,6 +700,7 @@ public void downloadBlob(String container, String name, File destination, Execut
>
> Futures.getUnchecked(Futures.allAsList(results));
>
> + raf.close();
> } catch (IOException e) {
> // cleanup, attempt to delete large file
> if (raf != null) {
Same here. If you close it in the finally block, there is no need to close it here too.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/ef80208da1149c5d1545d6cbf5c182491545ffa0#r77898439
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
> @@ -701,14 +702,11 @@ public void downloadBlob(String container, String name, File destination, Execut
> Futures.getUnchecked(Futures.allAsList(results));
>
> } catch (IOException e) {
> - // cleanup, attempt to delete large file
> - if (raf != null) {
> - try {
> - raf.close();
> - } catch (IOException e1) {}
> - }
> + Closeables2.closeQuietly(raf);
Good point, I'll submit a quick PR for it and one other small fix.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r78471081
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
> @@ -819,10 +817,7 @@ public void run() {
> }
> }
> // Finished writing results to stream
> - try {
> - output.close();
> - } catch (IOException e) {
> - }
> + Closeables2.closeQuietly(output);
> enclose the entire for block in a try/finally block
I merged the fixes for now, I will see if I can do a somewhat more complex refactoring in a separate PR later. I wanted to spend some time looking at reusing this code for BaseBlobstore first, before I spend too much time on it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r77907926
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
Thanks @nacx ! There's probably a few more things to patch up in this new code for the parallel stuff, I'll be working on it some more soon.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007#issuecomment-245413518
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -819,10 +817,7 @@ public void run() {
> }
> }
> // Finished writing results to stream
> - try {
> - output.close();
> - } catch (IOException e) {
> - }
> + Closeables2.closeQuietly(output);
Also, would it make sense to enclose the entire `for` block in a try/finally block, to make sur everything is always closed, without having to do it explicitly in the middle of the code and concrete catch blocks?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r77901189
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
> @@ -701,14 +702,11 @@ public void downloadBlob(String container, String name, File destination, Execut
> Futures.getUnchecked(Futures.allAsList(results));
>
> } catch (IOException e) {
> - // cleanup, attempt to delete large file
> - if (raf != null) {
> - try {
> - raf.close();
> - } catch (IOException e1) {}
> - }
> + Closeables2.closeQuietly(raf);
https://github.com/jclouds/jclouds/pull/1010
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r78502635
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Zack Shoylev <no...@github.com>.
Closed #1007.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007#event-781570031
Re: [jclouds/jclouds] Some handles were not properly closed (#1007)
Posted by Andrew Gaul <no...@github.com>.
> @@ -701,14 +702,11 @@ public void downloadBlob(String container, String name, File destination, Execut
> Futures.getUnchecked(Futures.allAsList(results));
>
> } catch (IOException e) {
> - // cleanup, attempt to delete large file
> - if (raf != null) {
> - try {
> - raf.close();
> - } catch (IOException e1) {}
> - }
> + Closeables2.closeQuietly(raf);
You can do what @nacx suggests with slightly more complicated resource management:
```java
RandomAccessFile raf = null;
File tmpFile = null;
try {
tempFile = new File(...);
raf = new RandomAccessFile(tempFile);
raf.write(...);
raf.close();
raf = null;
tempFile.renameTo(destinationFile);
tempFile = null;
} finally {
Closeables2.closeQuietly(raf);
if (tempFile != null) {
tempFile.delete();
}
}
```
This ensures that `destinationFile` is not overwritten unless the download succeeds.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1007/files/0003721c3e983e3f3c08824d86cbc7e5098f4eb4#r78420532