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

Do not printStackTrace on an exception you are going to rethrow #220

Merged
merged 3 commits into from
Dec 16, 2020

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 14, 2020

Noticed in jenkinsci/mercurial-plugin#149 that the AbortException gets a stack trace printed (which is wrong—it is a user error) even though https://github.com/jenkinsci/cloudbees-folder-plugin/blob/3afb5acfeeca5a89cf97edc88e8d497b327667c3/src/main/java/com/cloudbees/hudson/plugins/folder/computed/FolderComputation.java#L171-L174 was going to print it politely at the end of the branch indexing log.

You should only print a stack trace if you are not rethrowing an exception.

@@ -1509,7 +1509,7 @@ public void complete() throws IllegalStateException, IOException, InterruptedExc
} catch (InterruptedException | IOException x) {
throw x;
} catch (Exception x) {
x.printStackTrace(listener.error("Failed to create or update a subproject " + projectName));
printStackTrace(x, listener.error("Failed to create or update a subproject " + projectName));
Copy link
Member Author

Choose a reason for hiding this comment

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

Separate fix to use the Jenkins cause chaining.

@bitwiseman bitwiseman closed this Dec 16, 2020
@bitwiseman bitwiseman reopened this Dec 16, 2020
@jglick
Copy link
Member Author

jglick commented Dec 16, 2020

@bitwiseman bitwiseman reopened this 6 minutes ago

BTW you can just click Re-run on the Jenkins check.

@bitwiseman bitwiseman merged commit 6b0901b into jenkinsci:master Dec 16, 2020
@jglick jglick deleted the printStackTrace branch December 16, 2020 16:46
@jtnord
Copy link
Member

jtnord commented Aug 4, 2021

noting user (well test also) based regressions when indexing is interrupted. now there is no sign of what went wrong or where - the details of the InteruptedException are completely lost due to the special handling of the interuptedException in cloudbees-folders plugin

https://github.com/jenkinsci/cloudbees-folder-plugin/blob/07dd67fe36db681e7fcc3b73b117af3dc18f66b5/src/main/java/com/cloudbees/hudson/plugins/folder/computed/FolderComputation.java#L168-L171

@jglick
Copy link
Member Author

jglick commented Aug 4, 2021

What kind of interesting stack trace were you expecting from an InterruptedException? Is this something that “went wrong”?

@jtnord
Copy link
Member

jtnord commented Aug 4, 2021

it does not even need a stack trace, just "it was interrupted" will do (because at the moment all you see is ABORTED and you have no idea why). the stack is useful to know where if you do want more info, and that is available if you turn on FINEST logging, but first you would need to know - and given aborting is usually by a user, (or step if you are in a regular job) both of which are generally accompanied by a log to say why, this leaves the case of some for of InteruptedEx where you no longer have that log anymore and you are like 🤷

Given BitBucket branch source has an explicit test for this I can only assume that some part of the API Can throw this exception under some conditions (yes the test needs to be changed, but all you are left with in the log is aborted with no clue as to why)

@jglick
Copy link
Member Author

jglick commented Aug 5, 2021

just "it was interrupted" will do (because at the moment all you see is ABORTED and you have no idea why)

Well, ABORTED means something was interrupted. For Runs we have a CauseOfInterruption system which is not yet in ComputedFolder but could be added.

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

Successfully merging this pull request may close these issues.

3 participants