[
https://issues.apache.org/jira/browse/HTTPCLIENT-1044?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984002#action_12984002
]
Steffen Yount edited comment on HTTPCLIENT-1044 at 1/19/11 9:23 PM:
--------------------------------------------------------------------
Hi Oleg,
Thanks for considering my patch, and sorry for the long turn around in getting
back to you.
*** I think there is value in uniform application/non-application of the RFC's
rules here:
To demonstrate what I mean, I believe I can make a statement equally valid to
your first comment with respect to the GET method.
"While GET methods should be limited to actions of retrieval and be idempotent,
in real life often they are not. The current implementation may already be
breaking existing applications in very nasty ways."
Some common examples of GET method usage diverging from spec in this way: html
form submissions, RPC based APIs over HTTP, tracking pixel requests, JSONP APIs
(hack for cross domain scripting limitations).
I don't think the RFC's idempotence rules should be selectively respected on
some HTTP methods and not on others (without really good reasons). "Do or do
not" as Yoda would say. :-)
The current implementation retries GET, HEAD, DELETE, OPTIONS, and TRACE
assuming idempotence.
*** It is better to keep the default implementation more idiot-proof:
I agree with your assessment that assuming idempotence and unintentionally
retrying non-idempotent requests can break applications in "very nasty ways".
The nastiest of these ways are in debugging what went wrong, since a) the
failure modes that lead to retries are rarely replicated or tested by app
developers and b) many novice developers lack the insight to understand the
value of idempotent implementations and to identify idempotence/non-idempotence
related bugs.
Considering that automatic retries are optional as far as the RFC is concerned
and considering the two points above, I'm on board with your assessment that
the default behavior of the DefaultHttpRequestRetryHandler shouldn't be to
retry PUT methods automatically.
But, I also think this reasoning should be taken one step further in the
interest of uniformity and idiot-proofing: the default behavior shouldn't be
using RFC defined method idempotence to enable/disable automatic retries on any
of the HTTP methods.
*** Still, I think it would be great for the HttpClient libraries to provide an
out of the box HttpRequestRetryHandler that uses the RFC2616's rules for
HTTP method idempotence to enable/disable automatic retries.
Sure, it is true that I can use a custom implementation of
HttpRequestRetryHandler. But the supported behavior I'm asking for here isn't
something crazy. It's something that should work in apps that respect the
RFC2616's rules for HTTP method idempotence, and it is something that could be
provided out of the box.
*** I've provided 2 new alternative patches to make changes in this direction,
please let me know if either one is acceptable.
The changes in "HTTPCLIENT-1044_planB.patch" are:
1. Defaulting to not retry GET, HEAD, DELETE, OPTIONS, and TRACE based on
assumed idempotence.
2. Adding a check to prevent resending unrepeatable entities that have already
been sent once.
3. Removing the "requestSentRetryEnabled" flag which seemed to be included for
specifying entity repeatability and allowed both PUT and POST methods to be
resent.
4. Adding a "retryIdempotentMethods" flag in place of the
"requestSentRetryEnabled" flag. This flag allows GET, HEAD, PUT, DELETE,
OPTIONS, and TRACE methods to be resent. (default value: false)
The changes in "HTTPCLIENT-1044_planC.patch" are:
1. Defaulting to not retry GET, HEAD, DELETE, OPTIONS, and TRACE based on
assumed idempotence.
2. Adding a check to prevent resending unrepeatable entities that have already
been sent once.
3. Adding a new DefaultIdempotentHttpRequestRetryHandler class which does allow
GET, HEAD, PUT, DELETE, OPTIONS, and TRACE methods to be resent.
Cheers,
-Steffen
was (Author: steffenyount):
Hi Oleg,
Thanks for considering my patch, and sorry for the long turn around in getting
back to you.
h4. I think there is value in uniform application/non-application of the RFC's
rules here:
To demonstrate what I mean, I believe I can make a statement equally valid to
your first comment with respect to the GET method.
"While GET methods should be limited to actions of retrieval and be idempotent,
in real life often they are not. The current implementation may already be
breaking existing applications in very nasty ways."
Some common examples of GET method usage diverging from spec in this way: html
form submissions, RPC based APIs over HTTP, tracking pixel requests, JSONP APIs
(hack for cross domain scripting limitations).
I don't think the RFC's idempotence rules should be selectively respected on
some HTTP methods and not on others (without really good reasons). "Do or do
not" as Yoda would say. :-)
The current implementation retries GET, HEAD, DELETE, OPTIONS, and TRACE
assuming idempotence.
h4. It is better to keep the default implementation more idiot-proof:
I agree with your assessment that assuming idempotence and unintentionally
retrying non-idempotent requests can break applications in "very nasty ways".
The nastiest of these ways are in debugging what went wrong, since a) the
failure modes that lead to retries are rarely replicated or tested by app
developers and b) many novice developers lack the insight to understand the
value of idempotent implementations and to identify idempotence/non-idempotence
related bugs.
Considering that automatic retries are optional as far as the RFC is concerned
and considering the two points above, I'm on board with your assessment that
the default behavior of the DefaultHttpRequestRetryHandler shouldn't be to
retry PUT methods automatically.
But, I also think this reasoning should be taken one step further in the
interest of uniformity and idiot-proofing: the default behavior shouldn't be
using RFC defined method idempotence to enable/disable automatic retries on any
of the HTTP methods.
h4. Still, I think it would be great for the HttpClient libraries to provide an
out of the box HttpRequestRetryHandler that uses the RFC2616's rules for
HTTP method idempotence to enable/disable automatic retries.
Sure, it is true that I can use a custom implementation of
HttpRequestRetryHandler. But the supported behavior I'm asking for here isn't
something crazy. It's something that should work in apps that respect the
RFC2616's rules for HTTP method idempotence, and it is something that could be
provided out of the box.
h4. I've provided 2 new alternative patches to make changes in this direction,
please let me know if either one is acceptable.
The changes in "HTTPCLIENT-1044_planB.patch" are:
# Defaulting to not retry GET, HEAD, DELETE, OPTIONS, and TRACE based on
assumed idempotence.
# Adding a check to prevent resending unrepeatable entities that have already
been sent once.
# Removing the "requestSentRetryEnabled" flag which seemed to be included for
specifying entity repeatability and allowed both PUT and POST methods to be
resent.
# Adding a "retryIdempotentMethods" flag in place of the
"requestSentRetryEnabled" flag. This flag allows GET, HEAD, PUT, DELETE,
OPTIONS, and TRACE methods to be resent. (default value: false)
The changes in "HTTPCLIENT-1044_planC.patch" are:
# Defaulting to not retry GET, HEAD, DELETE, OPTIONS, and TRACE based on
assumed idempotence.
# Adding a check to prevent resending unrepeatable entities that have already
been sent once.
# Adding a new DefaultIdempotentHttpRequestRetryHandler class which does allow
GET, HEAD, PUT, DELETE, OPTIONS, and TRACE methods to be resent.
Cheers,
-Steffen
> DefaultHttpRequestRetryHandler is not handling PUT as an idempotent method
> for retries, though RFC2616 section 9.1.2 clearly defines it to be one.
> --------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HTTPCLIENT-1044
> URL: https://issues.apache.org/jira/browse/HTTPCLIENT-1044
> Project: HttpComponents HttpClient
> Issue Type: Bug
> Affects Versions: 4.0.3, 4.1.0
> Reporter: Steffen Yount
> Attachments: HTTPCLIENT-1044.patch, HTTPCLIENT-1044_planB.patch,
> HTTPCLIENT-1044_planC.patch
>
>
> See attached patch file for a fix:
> Fix treats PUT requests as idempotent, marking them to be retried when their
> enclosed HttpEntity is either null or repeatable.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
