Fix for #523 #570
Fix for #523 #570
Conversation
… will catch also java.lang.Error Catch also Error on some locations where IOException is catched, as the github lib sometimes will catch IOException and wrap it in an Error
|
Can someone please merge this |
|
can you link the code which wraps the exception in an error? That alreadys sound off and should be changed - IMHO. |
bjoernhaeuser
changed the title
| @@ -250,13 +250,13 @@ private void triggerPr(final GhprbTrigger trigger, | |||
| public void run() { | |||
| try { | |||
| trigger.handlePR(pr); | |||
| } catch (Exception e) { | |||
| } catch (Throwable th) { | |||
There was a problem hiding this comment.
In general, I've been told catching Throwable is a bad idea. Can a more seasoned Jenkins dev review this PR?
cc @jenkinsci/code-reviewers
There was a problem hiding this comment.
If the intention here is to catch Exception and Error, then check for those types and if it's not either of those types then rethrow throwable. I'm just concerned that we're swallowing all throwable.
There was a problem hiding this comment.
Also, a quote from the java.lang.Error javadoc:
An
Erroris a subclass ofThrowablethat indicates serious problems that a reasonable application should not try to catch.
I need an adult 🆘
There was a problem hiding this comment.
It's a bad style for sure. But, you know, throwing it from the code is a bad idea as well :(
There was a problem hiding this comment.
In practice the main things it is problematic to catch are OutOfMemoryError (generally no practical recovery, so does not make much difference either way); and ThreadDeath (you need to be sure to rethrow quickly at least).
There was a problem hiding this comment.
I'll open up a followup PR @jglick. I guess I merged too quickly.
There was a problem hiding this comment.
👍 since you do not have much choice here, but generally the mess with Errors in GitHub API should be cleaned up (https://github.com/kohsuke/github-api/search?utf8=%E2%9C%93&q=%22new+Error%22&type=). At least it makes sense to replace them by common runtime exceptions like IllegalStateException or whatever
| @@ -250,13 +250,13 @@ private void triggerPr(final GhprbTrigger trigger, | |||
| public void run() { | |||
| try { | |||
| trigger.handlePR(pr); | |||
| } catch (Exception e) { | |||
| } catch (Throwable th) { | |||
There was a problem hiding this comment.
It's a bad style for sure. But, you know, throwing it from the code is a bad idea as well :(
|
@bjoernhaeuser I'm headed to bed now. I trust @oleg-nenashev judgement on this one. When the PR finishes verifying do you mind merging? I'll get to it tomorrow if it's not merged so not a big deal if you don't have time. |
This issue is caused by the github lib on these occasions will catch an IOException and wrap it in a java.lang.Error. This plugin has a catch-all in the thread for Exceptions, but that does not catch an Error.
I've changed that to catch Throwable, which will catch both Error and Exception. I've also added some catching of Error higher up where it catches IOException.
Edit by @samrocketman: Fix for #523