palantir / gradle-baseline

A set of Gradle plugins that configure default code quality tools for developers.
Apache License 2.0
299 stars 136 forks source link

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

Open iamdanfox opened 4 years ago

iamdanfox commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

carterkozak commented 4 years ago

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.

alexdlm commented 2 years ago

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