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

[JENKINS-37566] - FindBugs - Cleanup issues in ExportTable implementation #157

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions src/main/java/hudson/remoting/ExportTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.io.PrintWriter;
Expand All @@ -39,6 +40,10 @@

import static java.util.logging.Level.*;
import javax.annotation.CheckForNull;
import javax.annotation.CheckReturnValue;
import javax.annotation.meta.When;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Manages unique ID for exported objects, and allows look-up from IDs.
Expand Down Expand Up @@ -225,11 +230,26 @@ static class Source extends Exception {
* Optional location that indicates where the actual call site was that triggered the activity,
* in case it was requested from the other side of the channel.
*/
@SuppressWarnings("ResultOfMethodCallIgnored")
Source(@CheckForNull Throwable callSite) {
super(callSite);
// force the computation of the stack trace in a Java friendly data structure,
// so that the call stack can be seen from the heap dump after the fact.
getStackTrace();
updateOurStackTraceCache();
}

// TODO: We export the objects frequently, The current approach ALWAYS leads
// to creation of two Stacktrace arrays in the memory: the original and the cloned one
// Throwable API. Throwable API allows to workaround it only by using a heavy printStackTrace() method.
// Approach #1: Maybe a manual implementation of getOurStackTrace() and local storage is preferable.
// Approach #2: Consider disabling this logic by default
/**
* Update the internal stacktrace cache.
* Forces the computation of the stack trace in a Java friendly data structure,
* so that the call stack can be seen from the heap dump after the fact.
* @return Cloned version of the inner cache.
*/
@CheckReturnValue(when = When.NEVER)
protected final StackTraceElement[] updateOurStackTraceCache() {
Copy link
Member

Choose a reason for hiding this comment

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

Just inline again and use @SuppressFBWarnings.

return getStackTrace();
}
}

Expand Down Expand Up @@ -257,10 +277,14 @@ public String toString() {

/**
* Captures the list of export, so that they can be unexported later.
*
* This is tied to a particular thread, so it only records operations
* on the current thread.
* The class is not serializable.
*/
@Restricted(NoExternalUse.class)
@SuppressFBWarnings(value = "SE_BAD_FIELD_INNER_CLASS",
justification = "ExportList is supposed to be serializable as ArrayList, but it is not. "
+ "The issue is ignored since the class does not belong to the public API")
public final class ExportList extends ArrayList<Entry> {
private final ExportList old;
private ExportList() {
Expand Down