Skip to content

Commit

Permalink
Merge pull request #368 from bitwiseman/connector-release
Browse files Browse the repository at this point in the history 
Fix connection caching - too many calls to release()
  • Loading branch information
@bitwiseman
bitwiseman authored Jan 13, 2021
2 parents 8abbb8f + af287f8 commit 95b7b1d
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 45 deletions.
28 changes: 17 additions & 11 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,11 @@ public static void release(@CheckForNull GitHub hub) {
synchronized (connections) {
GitHubConnection record = reverseLookup.get(hub);
if (record != null) {
record.release();
try {
record.release();
} catch (IOException e) {
LOGGER.log(WARNING, "There is a mismatch in connect and release calls.", e);
}
}
}
}
Expand Down Expand Up @@ -580,15 +584,17 @@ public static class UnusedConnectionDestroyer extends PeriodicWork {

@Override
public long getRecurrencePeriod() {
return TimeUnit.MINUTES.toMillis(2);
return TimeUnit.MINUTES.toMillis(5);
}

@Override
protected void doRun() throws Exception {
// free any connection unused for the last 5 minutes
long threshold = System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(5);
// Free any connection that is unused (zero refs)
// and has not been looked up or released for the last 30 minutes
long unusedThreshold = System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(30);

synchronized (connections) {
GitHubConnection.removeAllUnused(threshold);
GitHubConnection.removeAllUnused(unusedThreshold);
}
}
}
Expand Down Expand Up @@ -639,13 +645,13 @@ private static GitHubConnection connect(@NonNull ConnectionId connectionId, @Non
}


private void release() {
if (this.usageCount <= 1) {
this.usageCount = 0;
this.lastUsed = System.currentTimeMillis();
} else {
this.usageCount -= 1;
private void release() throws IOException {
if (this.usageCount <= 0) {
throw new IOException("Tried to release a GitHubConnection that should have no references.");
}

this.usageCount -= 1;
this.lastUsed = System.currentTimeMillis();
}

private static void removeAllUnused(long threshold) throws IOException {
Expand Down
5 changes: 1 addition & 4 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubBuildStatusNotification.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,8 @@ public void onEnterWaiting(Queue.WaitingItem wi) {
Computer.threadPoolForRemoting.submit(new Runnable() {
@Override
public void run() {
GitHub gitHub = null;
try {
gitHub = lookUpGitHub(job);
GitHub gitHub = lookUpGitHub(job);
try {
if (gitHub == null || gitHub.rateLimit().remaining < 8) {
// we are an optimization to signal commit status early, no point waiting for
Expand Down Expand Up @@ -269,8 +268,6 @@ public void run() {
"Could not update commit status to PENDING. Rate limit exhausted",
LOGGER.isLoggable(Level.FINE) ? e : null);
LOGGER.log(Level.FINE, null, e);
} finally {
Connector.release(gitHub);
}
}
});
Expand Down
21 changes: 9 additions & 12 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,33 +279,23 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
if (((PullRequestSCMHead)head).isMerge()) {
if (prRev.getMergeHash() == null) {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
}
prRev.validateMergeHash();
}
} else {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
}
} else {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
}

GHUser user = github.getUser(src.getRepoOwner());
if (user == null) {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
}
GHRepository repo = user.getRepository(src.getRepository());
if (repo == null) {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
}

Expand All @@ -328,10 +318,17 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch
rev = new AbstractGitSCMSource.SCMRevisionImpl(head, ref.getObject().getSha());
}
}
return new GitHubSCMFileSystem(github, repo, refName, rev);

// Instead of calling release in many case and skipping this one case
// Make another call to connect() for this case
// and always release the existing instance as part of finally block.
// The result is the same but with far fewer code paths calling release().
GitHub fileSystemGitHub = Connector.connect(apiUri, credentials);
return new GitHubSCMFileSystem(fileSystemGitHub, repo, refName, rev);
} catch (IOException | RuntimeException e) {
Connector.release(github);
throw e;
} finally {
Connector.release(github);
}
}
}
Expand Down
9 changes: 7 additions & 2 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMProbe.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

package org.jenkinsci.plugins.github_branch_source;

import com.cloudbees.plugins.credentials.common.StandardCredentials;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import jenkins.plugins.git.AbstractGitSCMSource;
Expand Down Expand Up @@ -57,8 +58,12 @@ class GitHubSCMProbe extends SCMProbe implements GitHubClosable {
private final String name;
private transient boolean open = true;

public GitHubSCMProbe(GitHub github, GHRepository repo, SCMHead head, SCMRevision revision) {
this.gitHub = github;
public GitHubSCMProbe(String apiUri,
StandardCredentials credentials,
GHRepository repo,
SCMHead head,
SCMRevision revision) throws IOException {
this.gitHub = Connector.connect(apiUri, credentials);
this.revision = revision;
this.repo = repo;
this.name = head.getName();
Expand Down
47 changes: 34 additions & 13 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,12 @@ public GHPermissionType fetch(String username) throws IOException, InterruptedEx
public SCMSourceCriteria.Probe create(@NonNull BranchSCMHead head,
@Nullable SCMRevisionImpl revisionInfo)
throws IOException, InterruptedException {
return new GitHubSCMProbe(github, ghRepository, head, revisionInfo);
return new GitHubSCMProbe(
apiUri,
credentials,
ghRepository,
head,
revisionInfo);
}
}, new CriteriaWitness(listener))) {
listener.getLogger().format("%n %d branches were processed (query completed)%n", count);
Expand Down Expand Up @@ -1035,7 +1040,7 @@ public SCMSourceCriteria.Probe create(@NonNull BranchSCMHead head,
for (final GHPullRequest pr : request.getPullRequests()) {
int number = pr.getNumber();
try {
retrievePullRequest(github, ghRepository, pr, strategies, request, listener);
retrievePullRequest(apiUri, credentials, ghRepository, pr, strategies, request, listener);
} catch (FileNotFoundException e) {
listener.getLogger().format("%n Error while processing pull request %d%n", number);
Functions.printStackTrace(e, listener.getLogger());
Expand Down Expand Up @@ -1091,7 +1096,12 @@ public SCMSourceCriteria.Probe create(@NonNull BranchSCMHead head,
public SCMSourceCriteria.Probe create(@NonNull GitHubTagSCMHead head,
@Nullable GitTagSCMRevision revisionInfo)
throws IOException, InterruptedException {
return new GitHubSCMProbe(github, ghRepository, head, revisionInfo);
return new GitHubSCMProbe(
apiUri,
credentials,
ghRepository,
head,
revisionInfo);
}
}, new CriteriaWitness(listener))) {
listener.getLogger()
Expand Down Expand Up @@ -1143,7 +1153,8 @@ private static void validatePullRequests(GitHubSCMSourceRequest request) {
}

private static void retrievePullRequest(
@NonNull final GitHub github,
final String apiUri,
final StandardCredentials credentials,
@NonNull final GHRepository ghRepository,
@NonNull final GHPullRequest pr,
@NonNull final Map<Boolean, Set<ChangeRequestCheckoutStrategy>> strategies,
Expand Down Expand Up @@ -1173,7 +1184,14 @@ private static void retrievePullRequest(

// PR details only needed for merge PRs
if (strategy == ChangeRequestCheckoutStrategy.MERGE) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
// The probe github will be closed along with the probe.
final GitHub gitHub = Connector.connect(apiUri, credentials);
try {
ensureDetailedGHPullRequest(pr, listener, gitHub, ghRepository);
}
finally {
Connector.release(gitHub);
}
}

if (request.process(new PullRequestSCMHead(
Expand All @@ -1190,8 +1208,9 @@ public SCMSourceCriteria.Probe create(@NonNull PullRequestSCMHead head,
if (!trusted) {
listener.getLogger().format(" (not from a trusted source)%n");
}
return new GitHubSCMProbe(github, ghRepository,
trusted ? head : head.getTarget(), null);
return new GitHubSCMProbe(
apiUri, credentials,
ghRepository, trusted ? head : head.getTarget(), null);
}
},
new SCMSourceRequest.LazyRevisionLambda<PullRequestSCMHead, SCMRevision, Void>() {
Expand All @@ -1201,7 +1220,7 @@ public SCMRevision create(@NonNull PullRequestSCMHead head,
@Nullable Void ignored)
throws IOException, InterruptedException {

return createPullRequestSCMRevision(pr, head, listener, github, ghRepository);
return createPullRequestSCMRevision(pr, head, listener, ghRepository);
}
},
new MergabilityWitness(pr, strategy, listener),
Expand Down Expand Up @@ -1395,7 +1414,7 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
if (head.isMerge()) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, head, listener, github, ghRepository);
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, head, listener, ghRepository);

switch (strategy) {
case MERGE:
Expand Down Expand Up @@ -1564,11 +1583,13 @@ protected SCMProbe createProbe(@NonNull SCMHead head, @CheckForNull final SCMRev
try {
String fullName = repoOwner + "/" + repository;
final GHRepository repo = github.getRepository(fullName);
return new GitHubSCMProbe(github, repo, head, revision);
return new GitHubSCMProbe(apiUri, credentials, repo, head, revision);
} catch (IOException | RuntimeException | Error e) {
Connector.release(github);
throw e;
} finally {
Connector.release(github);
}

}

@Override
Expand All @@ -1595,7 +1616,7 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc
if (prhead.isMerge()) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, ghRepository);
prRev.validateMergeHash();
return prRev;
} else if (head instanceof GitHubTagSCMHead) {
Expand All @@ -1620,7 +1641,7 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc
}
}

private static PullRequestSCMRevision createPullRequestSCMRevision(GHPullRequest pr, PullRequestSCMHead prhead, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {
private static PullRequestSCMRevision createPullRequestSCMRevision(GHPullRequest pr, PullRequestSCMHead prhead, TaskListener listener, GHRepository ghRepository) throws IOException, InterruptedException {
String baseHash = pr.getBase().getSha();
String prHeadHash = pr.getHead().getSha();
String mergeHash = null;
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMProbeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ void createProbeForPR(int number) throws IOException {

final GHRepository repo = github.getRepository("cloudbeers/yolo");
final PullRequestSCMHead head = new PullRequestSCMHead("PR-" + number, "cloudbeers", "yolo", "b", number, new BranchSCMHead("master"), new SCMHeadOrigin.Fork("rsandell"), ChangeRequestCheckoutStrategy.MERGE);
probe = new GitHubSCMProbe(github, repo,
head,
new PullRequestSCMRevision(head, "a", "b"));
probe = new GitHubSCMProbe("http://localhost:" + githubApi.port(), null,
repo,
head, new PullRequestSCMRevision(head, "a", "b"));
}

@Issue("JENKINS-54126")
Expand Down

0 comments on commit 95b7b1d

Please sign in to comment.