You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Svet <no...@github.com> on 2015/05/11 18:15:13 UTC
[jclouds] Don't retry unsafe HTTP methods in case of an IOException
(#744)
If an IOException is thrown during the execution of an HttpCommand retry only if the HTTP method doesn't have side effects (i.e. GET, HEAD). Otherwise the retry could cause unwanted side effects (i.e. creating and leaking multiple new nodes).
In my particular case Softlayer was timing out with the response and the retries eventually caused 6 machines to be created, but leaked, leaving them online and billing by the hour.
```
Caused by: java.net.SocketTimeoutException: Read timed out
at sun.reflect.GeneratedConstructorAccessor111.newInstance(Unknown Source)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:58)
at java.lang.reflect.Constructor.newInstance(Constructor.java:542)
at sun.net.www.protocol.http.HttpURLConnection$6.run(HttpURLConnection.java:1687)
at sun.net.www.protocol.http.HttpURLConnection$6.run(HttpURLConnection.java:1685)
at java.security.AccessController.doPrivileged(AccessController.java:333)
at sun.net.www.protocol.http.HttpURLConnection.getChainedException(HttpURLConnection.java:1683)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1256)
at java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
at com.ibm.net.ssl.www2.protocol.https.b.getResponseCode(b.java:63)
at org.jclouds.http.internal.JavaUrlHttpCommandExecutorService.invoke(JavaUrlHttpCommandExecutorService.java:105)
at org.jclouds.http.internal.JavaUrlHttpCommandExecutorService.invoke(JavaUrlHttpCommandExecutorService.java:66)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.invoke(BaseHttpCommandExecutorService.java:89)
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.reflect.FunctionalReflection$FunctionalInvocationHandler.handleInvocation(FunctionalReflection.java:117)
at com.google.common.reflect.AbstractInvocationHandler.invoke(AbstractInvocationHandler.java:87)
at com.sun.proxy.$Proxy134.createVirtualGuest(Unknown Source)
at org.jclouds.softlayer.compute.strategy.SoftLayerComputeServiceAdapter.createNodeWithGroupEncodedIntoName(SoftLayerComputeServiceAdapter.java:208)
at org.jclouds.compute.strategy.impl.AdaptingComputeServiceStrategies.createNodeWithGroupEncodedIntoName(AdaptingComputeServiceStrategies.java:195)
at org.jclouds.compute.strategy.impl.CreateNodesWithGroupEncodedIntoNameThenAddToSet$AddNode.call(CreateNodesWithGroupEncodedIntoNameThenAddToSet.java:79)
at org.jclouds.compute.strategy.impl.CreateNodesWithGroupEncodedIntoNameThenAddToSet$AddNode.call(CreateNodesWithGroupEncodedIntoNameThenAddToSet.java:63)
... 4 more
Caused by: java.net.SocketTimeoutException: Read timed out
at java.net.SocketInputStream.socketRead0(Native Method)
at java.net.SocketInputStream.read(SocketInputStream.java:164)
at java.net.SocketInputStream.read(SocketInputStream.java:134)
at com.ibm.jsse2.a.a(a.java:244)
at com.ibm.jsse2.a.a(a.java:35)
at com.ibm.jsse2.qc.a(qc.java:189)
at com.ibm.jsse2.qc.a(qc.java:513)
at com.ibm.jsse2.e.read(e.java:15)
at java.io.BufferedInputStream.fill(BufferedInputStream.java:247)
at java.io.BufferedInputStream.read1(BufferedInputStream.java:287)
at java.io.BufferedInputStream.read(BufferedInputStream.java:346)
at sun.net.www.http.HttpClient.parseHTTPHeader(HttpClient.java:717)
at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:663)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1335)
at com.ibm.net.ssl.www2.protocol.https.b.getInputStream(b.java:9)
at org.jclouds.http.internal.JavaUrlHttpCommandExecutorService.invoke(JavaUrlHttpCommandExecutorService.java:97)
... 16 more
```
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/744
-- Commit Summary --
* Don't retry unsafe HTTP methods in case of an IOException
-- File Changes --
M core/src/main/java/org/jclouds/http/handlers/BackoffLimitedRetryHandler.java (16)
M core/src/test/java/org/jclouds/http/handlers/BackoffLimitedRetryHandlerTest.java (59)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/744.patch
https://github.com/jclouds/jclouds/pull/744.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Ignasi Barrera <no...@github.com>.
This assumes that all providers have proper REST APIs. Several providers use only GET operations, and others use only POST methods describing an action. Removing the retries on POST requests globally may have more side effects than benefits.
Given that this can only be an issue in eventual consistent systems, would it make more sense to subclass the backoff strategy in the SoftLayer provider and have it configured to use that particular one, leaving the current one as-is for the rest of providers?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101236323
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Ignasi Barrera <no...@github.com>.
Oh yes, I see. What I'm not sure of the change is how we can guarantee this is applied consistently. Instead of applying the fix to a concrete implementation of a retry handler, would it make more sense to leave the handler unchanged, and move the `isIdempotent(command)` [up here](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L110)? This way the retry logic will be only considered when it makes sense, regardless of the retry handler configured in the provider.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101314904
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Andrea Turli <no...@github.com>.
> @@ -197,4 +221,29 @@ void testDisallowsExcessiveRetries() throws InterruptedException, IOException, S
> assertEquals(handler.shouldRetryRequest(command, response), false); // Failure 6
> }
>
> + private void testDisallowsExcessiveRetriesOnException(HttpCommand command) {
> + IOException response = new IOException();
> +
> + assertEquals(handler.shouldRetryRequest(command, response), true); // Failure 1
is there a way to refactor that using a loop? or something from assertj?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744/files#r30113568
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Svet <no...@github.com>.
@nacx Note that the changes are for the exception only case. This means that the client will send a request but won't receive the response. It doesn't matter whether the provider uses a proper REST API or not - if an exception occurs in the middle of a request, then it's not clear whether it can be retried safely. All providers could end up in the same scenario - retry a create machine call and get a leaked machine as a result.
I don't mind doing it for SL only, but looks like all providers could benefit. wdyt?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101248687
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Ignasi Barrera <no...@github.com>.
Just to be clear, the backoff retry handler is just the default one, and providers (several do) may override the handlers with custom ones to provide more fine-grained control when retrying upon service specific failures. They don't even have to extend the backoff limited retry handler. If we believe (I do) that this is a sane default behavior, I think we'd rather move the check up a level and don't force every retry handler to take care of that (and if it makes sense it is something we can easily make configurable per provider, if at any point we find it makes sense).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101316686
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Svet <no...@github.com>.
This is exactly what the current implementation does, no? It's not clear from the github diff, worth looking at the whole file.
The server error retry handler will call into another method - `public boolean shouldRetryRequest(HttpCommand command, HttpResponse response)` as opposed to the exception handler which calls into `public boolean shouldRetryRequest(HttpCommand command, IOException error)`. The latter implements `IOExceptionRetryHandler`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101275180
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Svet <no...@github.com>.
Addressed comments. Personally I see "safe" as a subset of "idempotent" methods, which includes `DELETE` and `PUT` in addition to `GET` and `HEAD`. I added them to the white-list as this makes the change differ in behaviour from before only for `POST` (and `PATCH`) which is where I had issues.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101221378
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/d4fa1159) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/41ff84bf). Thanks @neykov!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101815998
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Andrea Turli <no...@github.com>.
> @@ -90,13 +94,23 @@
> protected Logger logger = Logger.NULL;
>
> public boolean shouldRetryRequest(HttpCommand command, IOException error) {
> - return ifReplayableBackoffAndReturnTrue(command);
> + return isSafeToRetryOnException(command) && ifReplayableBackoffAndReturnTrue(command);
what about `isIdempotent(command)` instead of `isSafeToRetryOnException()` ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744/files#r30110560
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Svet <no...@github.com>.
Redid PR, moved changes to `BaseHttpCommandExecutorService`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101668005
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Ignasi Barrera <no...@github.com>.
Closed #744.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#event-304303170
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Svet <no...@github.com>.
Make sense, I'll update the PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101317635
Re: [jclouds] Don't retry unsafe HTTP methods in case of an
IOException (#744)
Posted by Ignasi Barrera <no...@github.com>.
Agree.
The problem here is that the `BackoffLimitedRetryHandler` is not only used to retry IO failures. It is also configured as the [default server error retry handler](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/handlers/DelegatingRetryHandler.java#L56).
Could it be a better approach to create a new [IOExceptionRetryHandler](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/IOExceptionRetryHandler.java) implementation (that could delegate to the backoff one), and configure it as [the default one](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/IOExceptionRetryHandler.java#L25)? This way we keep the current behavior for other uses of the backoff retry handler unchanged this new class would [only handle the IOExceptions](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L110) (which is what we're trying to achieve here). WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/744#issuecomment-101270297