You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2015/03/09 12:51:15 UTC
[jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
https://issues.apache.org/jira/browse/JCLOUDS-532
This commit moves the responsibility of closing the HTTP streams to the base class for the command execution services (which drivers inherit from), instead of being in a concrete implementation of a retry handler.
If the request has to be retried, we just close the stream and execute the request again. If the request cannot be retried and is not in the 2xx range, then we buffer and close the stream and pass it to the error handler. We could delegate the responsibility of closing the stream to the error handler without buffering it first (actually most, if not all, error handlers take care of closing the stream), but in the current implementation of the default driver, the error stream was buffered by default, so it seems safe to apply that behavior to the driver's base class, and remove that responsibility from each provider's error handler. Error streams shouldn't be big enough to present memory issues.
@andrewgaul This is a sensible change, specially for BlobStore providers, which make an extensive use of the response payloads. Could you run a smoke/live test on the common providers with the changes in this branch?
/cc @zack-shoylev
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/700
-- Commit Summary --
* JCLOUDS-532: Properly close HTTP streams
-- File Changes --
M core/src/main/java/org/jclouds/http/handlers/BackoffLimitedRetryHandler.java (2)
M core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java (13)
M core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java (16)
M core/src/test/java/org/jclouds/http/handlers/BackoffLimitedRetryHandlerTest.java (31)
A core/src/test/java/org/jclouds/http/internal/BaseHttpCommandExecutorServiceTest.java (185)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/700.patch
https://github.com/jclouds/jclouds/pull/700.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700
Re: [jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by Zack Shoylev <no...@github.com>.
The second approach seems much safer. Calling close should be sufficient unless I am missing something?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700#issuecomment-78122919
Re: [jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by Ignasi Barrera <no...@github.com>.
Closed #700.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700#event-249668658
Re: [jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by Ignasi Barrera <no...@github.com>.
OK, I've updated the pull request with a better approach. Now we don't buffer the error stream by default, and just pass the open stream to the error handler. It is up to it to close the stream, but after its execution we'll try to close it again (but won't fail if already closed or an exception is thrown while trying to close it).
This way we pass an open stream tot he error handlers and make sure it is always closed when needed.
My only doubt now is if just calling `close()` on the HTTP stream is enough or we'd better consume all the content before with some helper method such as `Strings2.toStringAndClose` or similar.
@andrewgaul is it safe to just close the stream? And, can you give another try to the tests with the new approach? Thanks for your feedback!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700#issuecomment-77967865
Re: [jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by Andrew Gaul <no...@github.com>.
@nacx Same successful results upon retesting the latest commit.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700#issuecomment-78090772
Re: [jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [ec63b55a](https://git1-us-west.apache.org/repos/asf?p=jclouds.git;a=commit;h=ec63b55a). Thanks @andrewgaul and @zack-shoylev!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700#issuecomment-78148456
Re: [jclouds/jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by vpanghal <no...@github.com>.
@nacx This seems to breaking swift delete operation. Swift Delete operation used to be Idempotent but after this change second delete operation is failing with these stack traces
`org.jclouds.http.HttpResponseException: java.io.IOException: stream is closed connecting to DELETE https://dal05.objectstorage.softlayer.net/v1/AUTH_0eb7ef10-6761-4384-ab47-9187592f30b1/cloud-testing/74f346838a7b550f HTTP/1.1
at org.jclouds.http.internal.BaseHttpCommandExecutorService.invoke(BaseHttpCommandExecutorService.java:114)
at org.jclouds.rest.internal.InvokeHttpMethod.invoke(InvokeHttpMethod.java:90)
at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:73)
at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:44)
at org.jclouds.rest.internal.DelegatesToInvocationFunction.handle(DelegatesToInvocationFunction.java:156)
at org.jclouds.rest.internal.DelegatesToInvocationFunction.invoke(DelegatesToInvocationFunction.java:123)
at com.sun.proxy.$Proxy67.removeObject(Unknown Source)
at org.jclouds.openstack.swift.blobstore.SwiftBlobStore.removeBlob(SwiftBlobStore.java:260)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at com.google.inject.internal.DelegatingInvocationHandler.invoke(DelegatingInvocationHandler.java:37)
at com.sun.proxy.$Proxy69.removeBlob(Unknown Source)
at com.maginatics.raptor.data.BlobStoreTest.testBlobRemoveIdempotence(BlobStoreTest.java:208)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:239)
at org.junit.rules.RunRules.evaluate(RunRules.java:20)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.apache.maven.surefire.junitcore.pc.Scheduler$1.run(Scheduler.java:393)
at org.apache.maven.surefire.junitcore.pc.InvokerStrategy.schedule(InvokerStrategy.java:54)
at org.apache.maven.surefire.junitcore.pc.Scheduler.schedule(Scheduler.java:352)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runners.Suite.runChild(Suite.java:128)
at org.junit.runners.Suite.runChild(Suite.java:27)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.apache.maven.surefire.junitcore.pc.Scheduler$1.run(Scheduler.java:393)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.RuntimeException: java.io.IOException: stream is closed
at com.google.common.base.Throwables.propagate(Throwables.java:160)
at org.jclouds.http.HttpUtils.toByteArrayOrNull(HttpUtils.java:127)
at org.jclouds.http.HttpUtils.closeClientButKeepContentStream(HttpUtils.java:159)
at org.jclouds.openstack.swift.handlers.ParseSwiftErrorFromHttpResponse.handleError(ParseSwiftErrorFromHttpResponse.java:49)
at org.jclouds.http.handlers.DelegatingErrorHandler.handleError(DelegatingErrorHandler.java:65)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.shouldContinue(BaseHttpCommandExecutorService.java:132)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.invoke(BaseHttpCommandExecutorService.java:101)
... 46 more
Caused by: java.io.IOException: stream is closed
at sun.net.www.protocol.http.HttpURLConnection$HttpInputStream.ensureOpen(HttpURLConnection.java:3308)
at sun.net.www.protocol.http.HttpURLConnection$HttpInputStream.read(HttpURLConnection.java:3333)
at sun.net.www.protocol.http.HttpURLConnection$HttpInputStream.read(HttpURLConnection.java:3328)
at com.google.common.io.ByteStreams.copy(ByteStreams.java:110)
at com.google.common.io.ByteStreams.toByteArray(ByteStreams.java:168)
at org.jclouds.http.HttpUtils.toByteArrayOrNull(HttpUtils.java:125)
... 51 more`
--
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/700#issuecomment-246494384
Re: [jclouds] JCLOUDS-532: Properly close HTTP streams (#700)
Posted by Andrew Gaul <no...@github.com>.
I have not yet reviewed this, but I successfully ran integration tests against atmos, aws-s3, azureblob, google-cloud-storage, and rackspace-cloudfiles-us.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/700#issuecomment-77943974