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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from Jun 19, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3a2040
fix: reset the thread's interrupt bit after catching InterruptedExcep…
Mar 20, 2020
5711c04
- Updated code comment to:
Mar 20, 2020
f8b6577
Fix bad merge
Jun 18, 2020
9bba5a2
formatted files with "mvn clean com.coveo:fmt-maven-plugin:format"
Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...e-http-client/src/main/java/com/google/api/client/http/HttpBackOffIOExceptionHandler.java
View file
Open in desktop
Expand Up @@ -97,6 +97,12 @@ public boolean handleIOException(HttpRequest request, boolean supportsRetry) thr
try {
return BackOffUtils.next(sleeper, backOff);
} catch (InterruptedException exception) {
<<<<<<< HEAD
Copy link
Contributor

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

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.

// Catching InterruptedException clears the thread interrupted bit.
Copy link
Contributor

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

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

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
Contributor

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

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
Contributor

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.

=======
// Mark thread as interrupted since we cannot throw InterruptedException here.
>>>>>>> e66dee68... fix: reset the thread's interrupt bit after catching InterruptedException (#1005)
Thread.currentThread().interrupt();
return false;
}
}
Expand Down
3 changes: 2 additions & 1 deletion ...ient/src/main/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandler.java
View file
Open in desktop
Expand Up @@ -128,7 +128,8 @@ public boolean handleResponse(HttpRequest request, HttpResponse response, boolea
try {
return BackOffUtils.next(sleeper, backOff);
} catch (InterruptedException exception) {
// ignore
// Mark thread as interrupted since we cannot throw InterruptedException here.
Thread.currentThread().interrupt();
}
}
return false;
Expand Down
1 change: 1 addition & 0 deletions google-http-client/src/main/java/com/google/api/client/http/HttpIOExceptionHandler.java
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@
* try {
* return BackOffUtils.next(sleeper, backOff);
* } catch (InterruptedException exception) {
* Thread.currentThread().interrupt();
* return false;
* }
* }
Expand Down
30 changes: 30 additions & 0 deletions ...tp-client/src/test/java/com/google/api/client/http/HttpBackOffIOExpcetionHandlerTest.java
View file
Open in desktop
Expand Up @@ -17,7 +17,9 @@
import com.google.api.client.testing.util.MockBackOff;
import com.google.api.client.testing.util.MockSleeper;
import com.google.api.client.util.BackOff;
import com.google.api.client.util.Sleeper;
import java.io.IOException;
import java.util.concurrent.atomic.AtomicBoolean;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -46,4 +48,32 @@ public void subsetHandle(long count, long millis, boolean retrySupported, BackOf
}
assertEquals(count, sleeper.getCount());
}

public void testHandleIOException_returnsFalseAndThreadRemainsInterrupted_whenSleepIsInterrupted()
throws Exception {
final AtomicBoolean stillInterrupted = new AtomicBoolean(false);
Thread runningThread = new Thread() {
@Override
public void run() {
HttpBackOffIOExceptionHandler testTarget =
new HttpBackOffIOExceptionHandler(
new MockBackOff().setBackOffMillis(Long.MAX_VALUE) // Sleep until we interrupt it.
.setMaxTries(1))
.setSleeper(Sleeper.DEFAULT); // Needs to be a real sleeper so we can interrupt it.

try {
testTarget.handleIOException(null, /* retrySupported= */ true);
} catch (Exception ignored) {
}
stillInterrupted.set(Thread.currentThread().isInterrupted());
}
};
runningThread.start();
// Give runningThread some time to start.
Thread.sleep(500L);
runningThread.interrupt();
runningThread.join();

assertTrue(stillInterrupted.get());
}
}
31 changes: 31 additions & 0 deletions .../src/test/java/com/google/api/client/http/HttpBackOffUnsuccessfulResponseHandlerTest.java
View file
Open in desktop
Expand Up @@ -18,7 +18,9 @@
import com.google.api.client.testing.util.MockBackOff;
import com.google.api.client.testing.util.MockSleeper;
import com.google.api.client.util.BackOff;
import com.google.api.client.util.Sleeper;
import java.io.IOException;
import java.util.concurrent.atomic.AtomicBoolean;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -67,4 +69,33 @@ private void subsetHandleResponse(
}
assertEquals(count, sleeper.getCount());
}

public void testHandleResponse_returnsFalseAndThreadRemainsInterrupted_whenSleepIsInterrupted()
throws Exception {
final AtomicBoolean stillInterrupted = new AtomicBoolean(false);
Thread runningThread = new Thread() {
@Override
public void run() {
HttpBackOffUnsuccessfulResponseHandler testTarget =
new HttpBackOffUnsuccessfulResponseHandler(
new MockBackOff().setBackOffMillis(Long.MAX_VALUE) // Sleep until we interrupt it.
.setMaxTries(1))
.setSleeper(Sleeper.DEFAULT) // Needs to be a real sleeper so we can interrupt it.
.setBackOffRequired(BackOffRequired.ALWAYS);

try {
testTarget.handleResponse(null, null, /* retrySupported= */ true);
} catch (Exception ignored) {
}
stillInterrupted.set(Thread.currentThread().isInterrupted());
}
};
runningThread.start();
// Give runningThread some time to start.
Thread.sleep(500L);
runningThread.interrupt();
runningThread.join();

assertTrue(stillInterrupted.get());
}
}