Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 9f3f644

Browse filesBrowse files
mmitchemmitche
committed
Add some level of synchronization to the root of the API
This adds some synchronization to the maps at the root of the API to avoid duplicated calls to the actual GH REST API. Specifically this is targeted around the two maps, orgs and users. This fix makes the GHPRB jenkins plugin behave much better when there are lots of projects that could build for a specific repo (even if only a few are actually triggered) There are also a few fixes around GHUser and GHPullRequest * GHPullRequest was checking a field that may be null (merged_by) when determining whether to fetch details. An unmerged PR would make a bunch of Github API calls for each property accessed. * Where GHUser was returned in various objects, we weren't going through the caching mechanism at the root, so calls to APIs on GHUSer often resulted in new REST calls. Instead, return from the cache wherever possible.
1 parent a2f0837 commit 9f3f644
Copy full SHA for 9f3f644

File tree

Expand file treeCollapse file tree

8 files changed

+83
-43
lines changed
Filter options
Expand file treeCollapse file tree

8 files changed

+83
-43
lines changed

‎src/main/java/org/kohsuke/github/GHCommitPointer.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHCommitPointer.java
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ public class GHCommitPointer {
3939
* This points to the user who owns
4040
* the {@link #getRepository()}.
4141
*/
42-
public GHUser getUser() {
42+
public GHUser getUser() throws IOException {
43+
if (user != null) return user.root.getUser(user.getLogin());
4344
return user;
4445
}
4546

‎src/main/java/org/kohsuke/github/GHCommitStatus.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHCommitStatus.java
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ public String getDescription() {
4545
return description;
4646
}
4747

48-
public GHUser getCreator() {
48+
public GHUser getCreator() throws IOException {
49+
if (creator != null) return creator.root.getUser(creator.getLogin());
4950
return creator;
5051
}
5152

‎src/main/java/org/kohsuke/github/GHDeployment.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHDeployment.java
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
package org.kohsuke.github;
22

3-
3+
import java.io.IOException;
44
import java.net.URL;
55

66
public class GHDeployment extends GHObject {
@@ -41,7 +41,8 @@ public String getPayload() {
4141
public String getEnvironment() {
4242
return environment;
4343
}
44-
public GHUser getCreator() {
44+
public GHUser getCreator() throws IOException {
45+
if(creator != null) return root.getUser(creator.getLogin());
4546
return creator;
4647
}
4748
public String getRef() {

‎src/main/java/org/kohsuke/github/GHGist.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHGist.java
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ public class GHGist extends GHObject {
3838
/**
3939
* User that owns this Gist.
4040
*/
41-
public GHUser getOwner() {
42-
return owner;
41+
public GHUser getOwner() throws IOException {
42+
return root.getUser(owner.getLogin());
4343
}
4444

4545
public String getForksUrl() {

‎src/main/java/org/kohsuke/github/GHIssue.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHIssue.java
+6-5Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -225,15 +225,16 @@ protected String getIssuesApiRoute() {
225225
return "/repos/"+owner.getOwnerName()+"/"+owner.getName()+"/issues/"+number;
226226
}
227227

228-
public GHUser getAssignee() {
228+
public GHUser getAssignee() throws IOException {
229+
if (assignee != null) return owner.root.getUser(assignee.getLogin());
229230
return assignee;
230231
}
231232

232233
/**
233234
* User who submitted the issue.
234235
*/
235-
public GHUser getUser() {
236-
return user;
236+
public GHUser getUser() throws IOException {
237+
return owner.root.getUser(user.getLogin());
237238
}
238239

239240
/**
@@ -244,9 +245,9 @@ public GHUser getUser() {
244245
* even for an issue that's already closed. See
245246
* https://github.com/kohsuke/github-api/issues/60.
246247
*/
247-
public GHUser getClosedBy() {
248+
public GHUser getClosedBy() throws IOException {
248249
if(!"closed".equals(state)) return null;
249-
if(closed_by != null) return closed_by;
250+
if(closed_by != null) return owner.root.getUser(closed_by.getLogin());;
250251

251252
//TODO closed_by = owner.getIssue(number).getClosed_by();
252253
return closed_by;

‎src/main/java/org/kohsuke/github/GHMilestone.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHMilestone.java
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ public GHRepository getOwner() {
2727
return owner;
2828
}
2929

30-
public GHUser getCreator() {
31-
return creator;
30+
public GHUser getCreator() throws IOException {
31+
return root.getUser(creator.getLogin());
3232
}
3333

3434
public Date getDueOn() {

‎src/main/java/org/kohsuke/github/GHPullRequest.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GHPullRequest.java
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ public String getMergeCommitSha() throws IOException {
200200
* Depending on the original API call where this object is created, it may not contain everything.
201201
*/
202202
private void populate() throws IOException {
203-
if (merged_by!=null) return; // already populated
203+
if (mergeable_state != null) return; // already populated by id
204204

205205
root.retrieve().to(url, this).wrapUp(owner);
206206
}

‎src/main/java/org/kohsuke/github/GitHub.java

Copy file name to clipboardExpand all lines: src/main/java/org/kohsuke/github/GitHub.java
+65-29Lines changed: 65 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,10 @@ public class GitHub {
7777
*/
7878
/*package*/ final String encodedAuthorization;
7979

80-
private final Map<String,GHUser> users = new Hashtable<String, GHUser>();
81-
private final Map<String,GHOrganization> orgs = new Hashtable<String, GHOrganization>();
82-
80+
private final Map<String,GHUser> users;
81+
private final Map<String,GHOrganization> orgs;
82+
// Cache of myself object.
83+
private GHMyself myself;
8384
private final String apiUrl;
8485

8586
/*package*/ final RateLimitHandler rateLimitHandler;
@@ -139,6 +140,8 @@ public class GitHub {
139140
}
140141
}
141142

143+
users = new Hashtable<String, GHUser>();
144+
orgs = new Hashtable<String, GHOrganization>();
142145
this.rateLimitHandler = rateLimitHandler;
143146

144147
if (login==null && encodedAuthorization!=null)
@@ -226,7 +229,7 @@ public HttpConnector getConnector() {
226229
/**
227230
* Sets the custom connector used to make requests to GitHub.
228231
*/
229-
public void setConnector(HttpConnector connector) {
232+
public synchronized void setConnector(HttpConnector connector) {
230233
this.connector = connector;
231234
}
232235

@@ -276,56 +279,89 @@ public GHRateLimit getRateLimit() throws IOException {
276279
public GHMyself getMyself() throws IOException {
277280
requireCredential();
278281

279-
GHMyself u = retrieve().to("/user", GHMyself.class);
280-
281-
u.root = this;
282-
users.put(u.getLogin(), u);
282+
// This entire block is under synchronization to avoid the relatively common case
283+
// where a bunch of threads try to enter this code simultaneously. While we could
284+
// scope the synchronization separately around the map retrieval and update (or use a concurrent hash)
285+
// map, the point is to avoid making unnecessary GH API calls, which are expensive from
286+
// an API rate standpoint
287+
synchronized (this) {
288+
if (this.myself != null) return myself;
289+
290+
GHMyself u = retrieve().to("/user", GHMyself.class);
283291

284-
return u;
292+
u.root = this;
293+
this.myself = u;
294+
return u;
295+
}
285296
}
286297

287298
/**
288299
* Obtains the object that represents the named user.
289300
*/
290301
public GHUser getUser(String login) throws IOException {
291-
GHUser u = users.get(login);
292-
if (u == null) {
293-
u = retrieve().to("/users/" + login, GHUser.class);
294-
u.root = this;
295-
users.put(u.getLogin(), u);
302+
// This entire block is under synchronization to avoid the relatively common case
303+
// where a bunch of threads try to enter this code simultaneously. While we could
304+
// scope the synchronization separately around the map retrieval and update (or use a concurrent hash
305+
// map), the point is to avoid making unnecessary GH API calls, which are expensive from
306+
// an API rate standpoint
307+
synchronized (users) {
308+
GHUser u = users.get(login);
309+
if (u == null) {
310+
u = retrieve().to("/users/" + login, GHUser.class);
311+
u.root = this;
312+
users.put(u.getLogin(), u);
313+
}
314+
return u;
296315
}
297-
return u;
298316
}
299317

300-
318+
301319
/**
302320
* clears all cached data in order for external changes (modifications and del
303321
*/
304322
public void refreshCache() {
305-
users.clear();
306-
orgs.clear();
323+
synchronized (users) {
324+
users.clear();
325+
}
326+
synchronized (orgs) {
327+
orgs.clear();
328+
}
307329
}
308330

309331
/**
310332
* Interns the given {@link GHUser}.
311333
*/
312334
protected GHUser getUser(GHUser orig) throws IOException {
313-
GHUser u = users.get(orig.getLogin());
314-
if (u==null) {
315-
orig.root = this;
316-
users.put(orig.getLogin(),orig);
317-
return orig;
335+
// This entire block is under synchronization to avoid the relatively common case
336+
// where a bunch of threads try to enter this code simultaneously. While we could
337+
// scope the synchronization separately around the map retrieval and update (or use a concurrent hash
338+
// map), the point is to avoid making unnecessary GH API calls, which are expensive from
339+
// an API rate standpoint
340+
synchronized (users) {
341+
GHUser u = users.get(orig.getLogin());
342+
if (u==null) {
343+
orig.root = this;
344+
users.put(orig.getLogin(),orig);
345+
return orig;
346+
}
347+
return u;
318348
}
319-
return u;
320349
}
321350

322351
public GHOrganization getOrganization(String name) throws IOException {
323-
GHOrganization o = orgs.get(name);
324-
if (o==null) {
325-
o = retrieve().to("/orgs/" + name, GHOrganization.class).wrapUp(this);
326-
orgs.put(name,o);
352+
// This entire block is under synchronization to avoid the relatively common case
353+
// where a bunch of threads try to enter this code simultaneously. While we could
354+
// scope the synchronization separately around the map retrieval and update (or use a concurrent hash
355+
// map), the point is to avoid making unnecessary GH API calls, which are expensive from
356+
// an API rate standpoint
357+
synchronized (orgs) {
358+
GHOrganization o = orgs.get(name);
359+
if (o==null) {
360+
o = retrieve().to("/orgs/" + name, GHOrganization.class).wrapUp(this);
361+
orgs.put(name,o);
362+
}
363+
return o;
327364
}
328-
return o;
329365
}
330366

331367
/**

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.