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

fix: restore the thread's interrupted status after catching InterruptedException (#1005) #1006

Merged
merged 4 commits into from Jun 19, 2020

Conversation

micheldavid
Copy link
Contributor

@micheldavid micheldavid commented Mar 20, 2020

Fixes #1005

@micheldavid micheldavid requested a review from as a code owner Mar 20, 2020
@googlebot googlebot added the cla: yes label Mar 20, 2020
@@ -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.
Copy link
Collaborator

@elharo elharo Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@chanseokoh chanseokoh Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@chanseokoh chanseokoh Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@elharo elharo Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help if this had a test demonstrating both that the bug is real and that this patch fixes it.

Copy link
Contributor Author

@micheldavid micheldavid Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@elharo elharo Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@elharo elharo added the kokoro:run label Mar 20, 2020
@kokoro-team kokoro-team removed the kokoro:run label Mar 20, 2020
@elharo
Copy link
Collaborator

@elharo elharo commented Apr 24, 2020

This might help with testing:

https://github.com/awaitility/awaitility

Mark thread as interrupted since we cannot throw InterruptedException here.

- Also setting thread interrupted in
HttpBackOffUnsuccessfulResponseHandler.

- Added tests for both HttpBackOffIOExceptionHandler and
HttpBackOffUnsuccessfulResponseHandler.
@micheldavid
Copy link
Contributor Author

@micheldavid micheldavid commented Jun 18, 2020

Rebased and added tests. I also re-interrupted the thread in HttpBackOffUnsuccessfulResponseHandler.

I noticed HttpRequest is swallowing an InterruptedException too, but that code depends on BackOffPolicy which javadoc says is scheduled to be removed in 1.18 so I did not bother fixing it.

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
Copy link
Collaborator

@elharo elharo Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad merge

Copy link
Contributor Author

@micheldavid micheldavid Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops... cant believe this got in here without me noticing.
fixed. ptal.

@elharo elharo added kokoro:force-run kokoro:run labels Jun 18, 2020
@kokoro-team kokoro-team removed kokoro:run kokoro:force-run labels Jun 18, 2020
@micheldavid micheldavid changed the title Resetting the Thread's interrupt bit after catching InterruptedException fix: restore the thread's interrupt status after catching InterruptedException (#1005) Jun 18, 2020
@micheldavid micheldavid changed the title fix: restore the thread's interrupt status after catching InterruptedException (#1005) fix: restore the thread's interrupted status after catching InterruptedException (#1005) Jun 18, 2020
elharo
elharo approved these changes Jun 18, 2020
@micheldavid
Copy link
Contributor Author

@micheldavid micheldavid commented Jun 18, 2020

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.

@elharo
Copy link
Collaborator

@elharo elharo commented Jun 18, 2020

yes, please run the plugin

@elharo elharo added the kokoro:force-run label Jun 19, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Jun 19, 2020
@micheldavid
Copy link
Contributor Author

@micheldavid micheldavid commented Jun 19, 2020

Now the linter is failing on the google-http-client-bom project, which doesnt seem to have any code...

From https://source.cloud.google.com/results/invocations/7a0f7d52-8162-4c7b-b711-f7c90e073a42/targets/cloud-devrel%2Fclient-libraries%2Fjava%2Fgoogle-http-java-client%2Fpresubmit%2Flint/log

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.
I feel like these formatting issues should be resolved in a separate pull request.

@chingor13
Copy link
Collaborator

@chingor13 chingor13 commented Jun 19, 2020

Strange, I don't recall us running the code formatter on this repo. Looking into it

@chingor13
Copy link
Collaborator

@chingor13 chingor13 commented Jun 19, 2020

We can fix the linting issue separately.

@chingor13 chingor13 merged commit 0a73a46 into googleapis:master Jun 19, 2020
9 of 10 checks passed
@micheldavid micheldavid deleted the fixinterrupt branch Jun 19, 2020
@micheldavid
Copy link
Contributor Author

@micheldavid micheldavid commented Jun 19, 2020

Thanks Jeff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
6 participants