Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request retrying on InterruptedIOException #1146

Closed
DanielNovak opened this issue Nov 11, 2014 · 0 comments · Fixed by #1257
Closed

Request retrying on InterruptedIOException #1146

DanielNovak opened this issue Nov 11, 2014 · 0 comments · Fixed by #1257
Labels
bug Bug in existing code
Milestone

Comments

@DanielNovak
Copy link

OkHttp 1.5.3 added basic support for Thread.interrupt() in commit 664b65d. This throws an InterruptedIOException but this is then handled by OkHttp's http retry engine in the catch block of HttpURLConnectionImpl.execute() method and the request is retried - which I think should not happen, because I deliberately interrupted the Thread and IO should stop (based on the commit from @swankjesse). I put logging inside OkHttp to verify that the same request is retried under some circumstances after calling Thread.interrupt() and the execute() catch block is indeed handling the InterruptedIOException.

@swankjesse swankjesse added the bug Bug in existing code label Nov 16, 2014
@swankjesse swankjesse added this to the 2.2 milestone Nov 16, 2014
swankjesse pushed a commit that referenced this issue Dec 30, 2014
nfuller added a commit to nfuller/okhttp that referenced this issue Aug 6, 2015
Related to issue square#1790.

If there is a *connection* timeout the next route should
be tried. Timeouts during a request/response should
probably not be retried.

The timeout/interrupt behavior was changed in PR square#1257
for issue square#1146. This modified both connection and
request/response behavior. The issue reported was actually
about the use of Thread.interrupt(), not timeouts, but the
behavior change modified both behavior for interrupt and
timeout.

PR square#1388 split the handling of exceptions so that separate
code now handles connection errors from that which handles
request/response errors. It faithfully kept the behavior
from PR square#1257.

The modification of the timeout behavior during *connection*
caused problems on Android. Now that the two types of error
handling are split it's possible to deal with them
differently and return the connection behavior to trying
the next route. This does not modified behavior during
request/response.

A test has been added for timeouts. It does not seem to be
possible to interrupt a Socket.connect() call with
Thread.interrupt() on either OpenJDK or Android so a
placeholder has been implemented. If it ever is implemented
a Thread.interrupt() during connect will not be retried.
nfuller added a commit to nfuller/okhttp that referenced this issue Aug 6, 2015
Related to issue square#1790.

If there is a *connection* timeout the next route should
be tried. Timeouts during a request/response should
probably not be retried.

The timeout/interrupt behavior was changed in PR square#1257
for issue square#1146. This modified both connection and
request/response behavior. The issue reported was actually
about the use of Thread.interrupt(), not timeouts, but the
behavior change modified both behavior for interrupt and
timeout.

PR square#1388 split the handling of exceptions so that separate
code now handles connection errors from that which handles
request/response errors. It faithfully kept the behavior
from PR square#1257.

The modification of the timeout behavior during *connection*
caused problems on Android. Now that the two types of error
handling are split it's possible to deal with them
differently and return the connection behavior to trying
the next route. This does not modify behavior during
request/response.

A test has been added for timeouts. It does not seem to be
possible to interrupt a Socket.connect() call with
Thread.interrupt() on either OpenJDK or Android so a
placeholder has been implemented. If it ever is implemented
a Thread.interrupt() during connect will not be retried.
nfuller added a commit to nfuller/okhttp that referenced this issue Aug 25, 2015
Related to issue square#1790.

If there is a *connection* timeout the next route should
be tried. Timeouts during a request/response should
probably not be retried.

The timeout/interrupt behavior was changed in PR square#1257
for issue square#1146. This modified both connection and
request/response behavior. The issue reported was actually
about the use of Thread.interrupt(), not timeouts, but the
behavior change modified both behavior for interrupt and
timeout.

PR square#1388 split the handling of exceptions so that separate
code now handles connection errors from that which handles
request/response errors. It faithfully kept the behavior
from PR square#1257.

The modification of the timeout behavior during *connection*
caused problems on Android. Now that the two types of error
handling are split it's possible to deal with them
differently and return the connection behavior to trying
the next route. This does not modify behavior during
request/response.

A test has been added for timeouts. It does not seem to be
possible to interrupt a Socket.connect() call with
Thread.interrupt() on either OpenJDK or Android so a
placeholder has been implemented. If it ever is implemented
a Thread.interrupt() during connect will not be retried.
nfuller added a commit to nfuller/okhttp that referenced this issue Sep 2, 2015
Related to issue square#1790.

If there is a *connection* timeout the next route should
be tried. Timeouts during a request/response should
probably not be retried.

The timeout/interrupt behavior was changed in PR square#1257
for issue square#1146. This modified both connection and
request/response behavior. The issue reported was actually
about the use of Thread.interrupt(), not timeouts, but the
behavior change modified both behavior for interrupt and
timeout.

PR square#1388 split the handling of exceptions so that separate
code now handles connection errors from that which handles
request/response errors. It faithfully kept the behavior
from PR square#1257.

The modification of the timeout behavior during *connection*
caused problems on Android. Now that the two types of error
handling are split it's possible to deal with them
differently and return the connection behavior to trying
the next route. This does not modify behavior during
request/response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants