fix: restore the thread's interrupted status after catching InterruptedException (#1005) #1006
Conversation
googlebot
added
the
cla: yes
micheldavid
force-pushed
the
fixinterrupt
branch
from
908866f
to
1ae169e
Compare
| @@ -97,6 +97,8 @@ public boolean handleIOException(HttpRequest request, boolean supportsRetry) thr | |||
| try { | |||
| return BackOffUtils.next(sleeper, backOff); | |||
| } catch (InterruptedException exception) { | |||
| // Catching InterruptedException clears the thread interrupted bit. | |||
There was a problem hiding this comment.
I could well be wrong here. I usually am when it comes to Thread arcana, but reading that article and the docs, this only seems necessary when you call interrupted(), not when you simply catch InterruptedException. That is, this code does not clear the interrupted bit. Am I missing something?
There was a problem hiding this comment.
This may well be the right thing to do.
Unless this code has control, authority, and context or institutes a policy so that eventually it can make the current interrupted thread die (or decide not to terminate the thread as its normal operation), or unless this code re-throws InterruptedException, or unless it is really correct that this code is designed to consume thread interruption and exit by returning false–which seems unlikely by lacking a comment, I think it should restore the interrupted bit, so that other parts of the code (someone higher in the call stack) can be made aware that this thread is interrupted and act accordingly.
https://stackoverflow.com/questions/2523721/why-do-interruptedexceptions-clear-a-threads-interrupted-status
https://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-in-a-catch-interruptexception-block
But I think the comment // Catching ... clears the thread interrupted bit. is unnecessary.
There was a problem hiding this comment.
And I actually think the comment "Catching InterruptedException clears the thread interrupted bit" is incorrect and misleading. It's not the act of catching it. Rather, I believe it is just that the interrupted bit is already cleared (and should by convention) when InterruptedException is thrown. From https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html,
By convention, any method that exits by throwing an InterruptedException clears interrupt status when it does so.
There was a problem hiding this comment.
It would help if this had a test demonstrating both that the bug is real and that this patch fixes it.
There was a problem hiding this comment.
Thanks for the quick response and thoughtful discussion.
Sorry about the delay. I havent had spare cycles to work on this recently...
I will write a test, but note that it is difficult to predict threading behavior so the test may be flaky at times, depending on cpu speed and available resources.
There was a problem hiding this comment.
Testing thread issues is extremely tricky. A flaky test can be worse than no test. This might need an integration test that doesn't run as part of the normal suite or some other magic.
kokoro-team
removed
the
kokoro:run
|
This might help with testing: |
micheldavid
force-pushed
the
fixinterrupt
branch
from
1ae169e
to
e3a2040
Compare
Mark thread as interrupted since we cannot throw InterruptedException here. - Also setting thread interrupted in HttpBackOffUnsuccessfulResponseHandler. - Added tests for both HttpBackOffIOExceptionHandler and HttpBackOffUnsuccessfulResponseHandler.
|
Rebased and added tests. I also re-interrupted the thread in I noticed HttpRequest is swallowing an InterruptedException too, but that code depends on BackOffPolicy which javadoc says is A bit more literature around interrupted exceptions: go/interrupted-exception (internal) and http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html |
| @@ -97,6 +97,12 @@ public boolean handleIOException(HttpRequest request, boolean supportsRetry) thr | |||
| try { | |||
| return BackOffUtils.next(sleeper, backOff); | |||
| } catch (InterruptedException exception) { | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
oops... cant believe this got in here without me noticing.
fixed. ptal.
elharo
added
kokoro:force-run
kokoro-team
removed
kokoro:run
micheldavid
changed the title
micheldavid
changed the title
|
Thanks for the quick turn around. I see a lot of files I did not change are failing the code format lint pass. I did change some of them though. Should I fix it with "mvn com.coveo:fmt-maven-plugin:format"? Maybe fix only the ones I changed? I also do not have write access, so I will need someone to merge this in. |
|
yes, please run the plugin |
elharo
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
Now the linter is failing on the Failed to execute goal com.coveo:fmt-maven-plugin:2.10:check (default-cli) on project google-http-client-bom Is this normal? The same seems to happen in master. |
|
Strange, I don't recall us running the code formatter on this repo. Looking into it |
|
We can fix the linting issue separately. |
|
Thanks Jeff! |
🤖 I have created a release \*beep\* \*boop\* --- ## [1.36.0](https://www.github.com/googleapis/google-http-java-client/compare/v1.35.0...v1.36.0) (2020-06-30) ### Features * add Android 19 compatible FileDataStoreFactory implementation ([#1070](https://www.github.com/googleapis/google-http-java-client/issues/1070)) ([1150acd](https://www.github.com/googleapis/google-http-java-client/commit/1150acd38aa3139eea4f2f718545c20d2493877e)) ### Bug Fixes * restore the thread's interrupted status after catching InterruptedException ([#1005](https://www.github.com/googleapis/google-http-java-client/issues/1005)) ([#1006](https://www.github.com/googleapis/google-http-java-client/issues/1006)) ([0a73a46](https://www.github.com/googleapis/google-http-java-client/commit/0a73a4628b6ec4420db6b9cdbcc68899f3807c5b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Fixes #1005