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

Print stack trace in logical order #1485

Merged
merged 14 commits into from Jan 28, 2017

Conversation

6 participants
@jglick
Member

jglick commented Dec 10, 2014

Follow suggestion from JDK-6507809 to print stack traces in a readable order.

  • handle exceptions which override printStackTrace
  • handle Java 7 suppressed exceptions
  • handle circular references
  • use system linefeed
  • complete unit tests
  • replace existing calls to Throwable.printStackTrace
  • use from support-core: jenkinsci/support-core-plugin#96

@reviewbybees

jglick added some commits Nov 6, 2013

Follow suggestion from JDK-6507809 [1] to print stack traces in a rea…
…dable order.

Would still need to handle some corner cases; write unit tests; and replace existing calls to Throwable.printStackTrace.

[1] https://bugs.openjdk.java.net/browse/JDK-6507809
Show outdated Hide outdated core/src/main/java/hudson/Functions.java
summary = summary.substring(0, summary.length() - suffix.length());
}
}
s.append(summary).append('\n');

This comment has been minimized.

@jtnord

jtnord Dec 11, 2014

Member

this changes the behaviour as e.printStackTrace() will use the platform line separator -
should this not be the system linefeed by default?

@jtnord

jtnord Dec 11, 2014

Member

this changes the behaviour as e.printStackTrace() will use the platform line separator -
should this not be the system linefeed by default?

This comment has been minimized.

@jglick

jglick Dec 11, 2014

Member

Probably yes.

@jglick

jglick Dec 11, 2014

Member

Probably yes.

@jglick jglick changed the title from [WiP] Print stack trace in logical order to Print stack trace in logical order Aug 4, 2015

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 3, 2016

Member

This would be so useful. @jglick Any plans to resurrect this PR?

Member

daniel-beck commented May 3, 2016

This would be so useful. @jglick Any plans to resurrect this PR?

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Dec 12, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

reviewbybees commented Dec 12, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Dec 12, 2016

Member

Seems to cause test failures; some TaskListener loggers apparently treat .print("foo\n") differently from .println("foo").

Member

jglick commented Dec 12, 2016

Seems to cause test failures; some TaskListener loggers apparently treat .print("foo\n") differently from .println("foo").

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Dec 12, 2016

Member

Indeed PrintWriter.newLine() does a flush, so these are not interchangeable. Will need to introduce convenience methods to replace original printStackTrace calls.

Member

jglick commented Dec 12, 2016

Indeed PrintWriter.newLine() does a flush, so these are not interchangeable. Will need to introduce convenience methods to replace original printStackTrace calls.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Dec 15, 2016

Member

Random test failure.

Member

jglick commented Dec 15, 2016

Random test failure.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Dec 15, 2016

Member

I don't think anyone cares as long as one of the two PR builds passes.

Member

daniel-beck commented Dec 15, 2016

I don't think anyone cares as long as one of the two PR builds passes.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jan 12, 2017

Member

@jglick Is this done and ready for review?

Member

daniel-beck commented Jan 12, 2017

@jglick Is this done and ready for review?

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Jan 12, 2017

Member

@daniel-beck yes I am awaiting review.

Member

jglick commented Jan 12, 2017

@daniel-beck yes I am awaiting review.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Jan 12, 2017

Member

🐝

Member

stephenc commented Jan 12, 2017

🐝

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick
Member

jglick commented Jan 12, 2017

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Jan 12, 2017

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

reviewbybees commented Jan 12, 2017

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

* @param pw the log
* @since FIXME
*/
public static void printStackTrace(@CheckForNull Throwable t, @Nonnull PrintWriter pw) {

This comment has been minimized.

@daniel-beck

daniel-beck Jan 16, 2017

Member

Name inconsistent with existing printThrowable, deliberate?

@daniel-beck

daniel-beck Jan 16, 2017

Member

Name inconsistent with existing printThrowable, deliberate?

This comment has been minimized.

@jglick

jglick Jan 26, 2017

Member

It is a replacement for an instance method in Throwable.

@jglick

jglick Jan 26, 2017

Member

It is a replacement for an instance method in Throwable.

@jglick jglick added ready-for-merge and removed needs-review labels Jan 26, 2017

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Jan 27, 2017

Member

🐝

Member

stephenc commented Jan 27, 2017

🐝

@daniel-beck daniel-beck merged commit a1cad0b into jenkinsci:master Jan 28, 2017

1 of 3 checks passed

Jenkins Looks like there's a problem with this pull request
Details
continuous-integration/jenkins/pr-merge This commit is scheduled to be built
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

daniel-beck added a commit that referenced this pull request Jan 30, 2017

@jglick jglick deleted the jglick:printStackTrace-JDK-6507809 branch Jan 30, 2017

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