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

FR: ensure users who catch InterruptedException also call Thread.currentThread().interrupt() #1043

Open
iamdanfox opened this issue Nov 14, 2019 · 4 comments · May be fixed by #1076
Open

FR: ensure users who catch InterruptedException also call Thread.currentThread().interrupt() #1043

iamdanfox opened this issue Nov 14, 2019 · 4 comments · May be fixed by #1076

Comments

@iamdanfox
Copy link
Contributor

iamdanfox commented Nov 14, 2019

What happened?

It's easy for people to write code like this with no failures:

} catch (InterruptedException e) {
  throw new RuntimeException(e);
}

Just rethrowing an exception like this is usually wrong because if the user is trying to abort, the calling thread will be unable to detect that an interruption happened and short-circuit.

What did you want to happen?

For the cases where people are re-throwing some unchecked exception, error-prone should suggest:

 } catch (InterruptedException e) {
+    Thread.currentThread().interrupt();
     throw new RuntimeException(e);
 }
@carterkozak
Copy link
Contributor

InterruptedExceptionSwallowed may do what we want, if memory serves it also suggests an instanceof check in the case of:

try {
    foo(); // throws Exception
} catch (Exception e) {
+    if (e instanceof InterruptedException) {
+         Thread.currentThread().interrupt();
+    }
    throw new RuntimeException(e);
}

@carterkozak
Copy link
Contributor

err, it looks like that hasn't made it into a release quite yet!

@carterkozak
Copy link
Contributor

The InterruptedExceptionSwallowed only appears to apply to catch blocks that catch a supertype of InterruptedException, not InterruptedException itself, I believe this is because catching InterruptedException describes the choice to reset the interrupted flag, however it's non-obvious that's the case. We should provide a comment to suppress this check along the lines of // interruption reset in a catch block.
I do much prefer the model of opting out of resetting the interrupted state as opposed to opting in, because it's not a discoverable mechanic.

carterkozak added a commit that referenced this issue Nov 26, 2019
I imagine we'll want to suppress some of these based on location
and context.
@carterkozak carterkozak linked a pull request Nov 27, 2019 that will close this issue
@alexdlm
Copy link

alexdlm commented Jun 2, 2022

Is there still any intention to implement this? It would be very valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants