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

Trace Stapler request processing #148

Merged
merged 3 commits into from Oct 11, 2018

Conversation

Projects
None yet
2 participants
@daniel-beck
Copy link
Member

daniel-beck commented Oct 9, 2018

Upstream from jenkinsci/jenkins#3688

@daniel-beck daniel-beck referenced this pull request Oct 9, 2018

Merged

Add telemetry for Stapler dispatches #3688

1 of 3 tasks complete
@daniel-beck

This comment has been minimized.

Copy link
Member Author

daniel-beck commented Oct 10, 2018

3b3105f (plus a local versions:set) deployed as 1.255-telemetry-1-SNAPSHOT.

private static List<ApplicationTracer> getTracers() {
if (tracers == null) {
synchronized (ApplicationTracer.class) {
if (tracers == null) {

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

Double-checked locking. Delete the outer null check.

@daniel-beck daniel-beck requested a review from jglick Oct 11, 2018

@daniel-beck

This comment has been minimized.

Copy link
Member Author

daniel-beck commented Oct 11, 2018

@jglick
Copy link
Member

jglick left a comment

Not what I had in mind, but at least no longer apparently vulnerable to data corruption.

}
}

private static volatile List<ApplicationTracer> tracers;

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

Pointless for this to be volatile, since you have the synchronized block.

tracers = t;
}
}
return tracers;

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

This should really be inside the synchronized block. I think it is harmless to have it outside, since there will be a synchronization barrier before, but definitely not the best style.

private static List<ApplicationTracer> getTracers() {
synchronized (ApplicationTracer.class) {
if (tracers == null) {
List<ApplicationTracer> t = new ArrayList<>();

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

You do not need a separate variable. You can initialize tracers directly, since it is protected by a synchronization barrier.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Oct 11, 2018

FTR all you need is something like:

private static List<X> xs;
static synchronized List<X> xs() {
  if (xs == null) {
    xs = new ArrayList<>();
    // populate xs
  }
  return xs;
}

Simple and safe.

@jglick jglick dismissed their stale review Oct 11, 2018

no longer a hazard

@jglick jglick merged commit d33d285 into stapler:master Oct 11, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

jglick added a commit that referenced this pull request Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.