Skip to content

Commit ce9c34f

Browse files
gbhat618gbhat618
committed
Do not use static state variables in Extension class
1 parent 54fb24f commit ce9c34f

File tree

2 files changed

+44
-45
lines changed

2 files changed

+44
-45
lines changed

‎src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java

+11-10
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import com.google.common.annotations.VisibleForTesting;
1313

1414
import hudson.Extension;
15+
import hudson.ExtensionList;
1516
import hudson.model.AdministrativeMonitor;
1617
import hudson.model.Item;
1718
import jenkins.model.Jenkins;
@@ -57,7 +58,7 @@ String getLastDuplicateUrl() {
5758

5859
@Override
5960
public boolean isActivated() {
60-
return DuplicateEventsSubscriber.isDuplicateEventSeen();
61+
return ExtensionList.lookupSingleton(DuplicateEventsSubscriber.class).isDuplicateEventSeen();
6162
}
6263

6364
@Override
@@ -75,7 +76,7 @@ public void checkRequiredPermission() {
7576
public HttpResponse doGetLastDuplicatePayload() {
7677
Jenkins.get().checkPermission(Jenkins.SYSTEM_READ);
7778
JSONObject data;
78-
var lastDuplicate = DuplicateEventsSubscriber.getLastDuplicate();
79+
var lastDuplicate = ExtensionList.lookupSingleton(DuplicateEventsSubscriber.class).getLastDuplicate();
7980
if (lastDuplicate != null) {
8081
data = JSONObject.fromObject(lastDuplicate.ghSubscriberEvent().getPayload());
8182
} else {
@@ -102,7 +103,7 @@ public static final class DuplicateEventsSubscriber extends GHEventsSubscriber {
102103

103104
private static final Logger LOGGER = Logger.getLogger(DuplicateEventsSubscriber.class.getName());
104105

105-
private static Ticker ticker = Ticker.systemTicker();
106+
private Ticker ticker = Ticker.systemTicker();
106107
/**
107108
* Caches GitHub event GUIDs for 10 minutes to track recent events to detect duplicates.
108109
* <p>
@@ -114,21 +115,21 @@ public static final class DuplicateEventsSubscriber extends GHEventsSubscriber {
114115
* timestamp (assuming caffeine internally keeps long) takes 8 bytes; total of 44 bytes
115116
* per entry. So the maximum memory consumed by this cache is 24k * 44 = 1056k = 1.056 MB.
116117
*/
117-
private static final Cache<String, Object> EVENT_TRACKER = Caffeine.newBuilder()
118+
private final Cache<String, Object> EVENT_TRACKER = Caffeine.newBuilder()
118119
.maximumSize(24_000L)
119120
.expireAfterWrite(Duration.ofMinutes(10))
120121
.ticker(() -> ticker.read())
121122
.build();
122123
private static final Object DUMMY = new Object();
123124

124-
private static volatile TrackedDuplicateEvent lastDuplicate;
125+
private volatile TrackedDuplicateEvent lastDuplicate;
125126
public record TrackedDuplicateEvent(
126127
String eventGuid, Instant lastUpdated, GHSubscriberEvent ghSubscriberEvent) { }
127128
private static final Duration TWENTY_FOUR_HOURS = Duration.ofHours(24);
128129

129130
@VisibleForTesting
130131
@Restricted(NoExternalUse.class)
131-
static void setTicker(Ticker testTicker) {
132+
void setTicker(Ticker testTicker) {
132133
ticker = testTicker;
133134
}
134135

@@ -187,16 +188,16 @@ protected void onEvent(final GHSubscriberEvent event) {
187188
*
188189
* @return {@code true} if a duplicate was seen in the last 24 hours, {@code false} otherwise.
189190
*/
190-
public static boolean isDuplicateEventSeen() {
191+
public boolean isDuplicateEventSeen() {
191192
return lastDuplicate != null
192193
&& Duration.between(lastDuplicate.lastUpdated(), getNow()).compareTo(TWENTY_FOUR_HOURS) < 0;
193194
}
194195

195-
private static Instant getNow() {
196+
private Instant getNow() {
196197
return Instant.ofEpochSecond(0L, ticker.read());
197198
}
198199

199-
public static TrackedDuplicateEvent getLastDuplicate() {
200+
public TrackedDuplicateEvent getLastDuplicate() {
200201
return lastDuplicate;
201202
}
202203

@@ -206,7 +207,7 @@ public static TrackedDuplicateEvent getLastDuplicate() {
206207
*/
207208
@VisibleForTesting
208209
@Restricted(NoExternalUse.class)
209-
static Set<String> getEventCountsTracker() {
210+
Set<String> getEventCountsTracker() {
210211
return EVENT_TRACKER.asMap().keySet().stream()
211212
.filter(key -> EVENT_TRACKER.getIfPresent(key) != null)
212213
.collect(Collectors.toSet());

‎src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java

+33-35
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
33
import static org.hamcrest.MatcherAssert.assertThat;
44
import static org.hamcrest.Matchers.is;
55
import static org.hamcrest.Matchers.nullValue;
6-
import static org.jenkinsci.plugins.github.admin.GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.getEventCountsTracker;
7-
import static org.jenkinsci.plugins.github.admin.GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.getLastDuplicate;
8-
import static org.jenkinsci.plugins.github.admin.GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.isDuplicateEventSeen;
96

107
import java.time.Duration;
118
import java.time.Instant;
@@ -21,81 +18,82 @@
2118
@For(GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.class)
2219
public class GitHubDuplicateEventsMonitorUnitTest {
2320

24-
private final GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber subscriber
25-
= new GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber();
26-
2721
@Test
2822
public void onEventShouldTrackEventAndKeepTrackOfLastDuplicate() {
23+
var subscriber = new GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber();
24+
2925
var now = Instant.parse("2025-02-05T03:00:00Z");
3026
var after1Sec = Instant.parse("2025-02-05T03:00:01Z");
3127
var after2Sec = Instant.parse("2025-02-05T03:00:02Z");
3228
FakeTicker fakeTicker = new FakeTicker(now);
33-
GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.setTicker(fakeTicker);
29+
subscriber.setTicker(fakeTicker);
3430

35-
assertThat("lastDuplicate is null at first", getLastDuplicate(), is(nullValue()));
36-
assertThat("should not throw NPE", isDuplicateEventSeen(), is(false));
31+
assertThat("lastDuplicate is null at first", subscriber.getLastDuplicate(), is(nullValue()));
32+
assertThat("should not throw NPE", subscriber.isDuplicateEventSeen(), is(false));
3733
// send a null event
3834
subscriber.onEvent(new GHSubscriberEvent(null, "origin", GHEvent.PUSH, "payload"));
39-
assertThat("null event is not tracked", getEventCountsTracker().size(), is(0));
40-
assertThat("lastDuplicate is still null", getLastDuplicate(), is(nullValue()));
35+
assertThat("null event is not tracked", subscriber.getEventCountsTracker().size(), is(0));
36+
assertThat("lastDuplicate is still null", subscriber.getLastDuplicate(), is(nullValue()));
4137

4238
// at present
4339
subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload"));
44-
assertThat(getEventCountsTracker(), is(Set.of("1")));
45-
assertThat(getLastDuplicate(), is(nullValue()));
46-
assertThat(isDuplicateEventSeen(), is(false));
40+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1")));
41+
assertThat(subscriber.getLastDuplicate(), is(nullValue()));
42+
assertThat(subscriber.isDuplicateEventSeen(), is(false));
4743
subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload"));
48-
assertThat(getLastDuplicate(), is(nullValue()));
49-
assertThat(isDuplicateEventSeen(), is(false));
50-
assertThat(getEventCountsTracker(), is(Set.of("1", "2")));
44+
assertThat(subscriber.getLastDuplicate(), is(nullValue()));
45+
assertThat(subscriber.isDuplicateEventSeen(), is(false));
46+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2")));
5147
subscriber.onEvent(new GHSubscriberEvent(null, "origin", GHEvent.PUSH, "payload"));
52-
assertThat(getLastDuplicate(), is(nullValue()));
53-
assertThat(getEventCountsTracker(), is(Set.of("1", "2")));
54-
assertThat(isDuplicateEventSeen(), is(false));
48+
assertThat(subscriber.getLastDuplicate(), is(nullValue()));
49+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2")));
50+
assertThat(subscriber.isDuplicateEventSeen(), is(false));
5551

5652
// after a second
5753
fakeTicker.advance(Duration.ofSeconds(1));
5854
subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload"));
59-
assertThat(getLastDuplicate().eventGuid(), is("1"));
60-
assertThat(getLastDuplicate().lastUpdated(), is(after1Sec));
61-
assertThat(getEventCountsTracker(), is(Set.of("1", "2")));
62-
assertThat(isDuplicateEventSeen(), is(true));
55+
assertThat(subscriber.getLastDuplicate().eventGuid(), is("1"));
56+
assertThat(subscriber.getLastDuplicate().lastUpdated(), is(after1Sec));
57+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2")));
58+
assertThat(subscriber.isDuplicateEventSeen(), is(true));
6359

6460
// second occurrence for another event after 2 seconds
6561
fakeTicker.advance(Duration.ofSeconds(1));
6662
subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload"));
67-
assertThat(getLastDuplicate().eventGuid(), is("2"));
68-
assertThat(getLastDuplicate().lastUpdated(), is(after2Sec));
69-
assertThat(getEventCountsTracker(), is(Set.of("1", "2")));
70-
assertThat(isDuplicateEventSeen(), is(true));
63+
assertThat(subscriber.getLastDuplicate().eventGuid(), is("2"));
64+
assertThat(subscriber.getLastDuplicate().lastUpdated(), is(after2Sec));
65+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2")));
66+
assertThat(subscriber.isDuplicateEventSeen(), is(true));
7167

7268
// 24 hours has passed; note we already added 2 seconds/ so effectively 24h 2sec now.
7369
fakeTicker.advance(Duration.ofHours(24));
74-
assertThat(isDuplicateEventSeen(), is(false));
70+
assertThat(subscriber.isDuplicateEventSeen(), is(false));
7571
}
7672

7773
@Test
7874
public void checkOldEntriesAreExpiredAfter10Minutes() {
75+
var subscriber = new GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber();
76+
7977
var now = Instant.parse("2025-02-05T03:00:00Z");
8078
FakeTicker fakeTicker = new FakeTicker(now);
81-
GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.setTicker(fakeTicker);
79+
subscriber.setTicker(fakeTicker);
8280

8381
// at present
8482
subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload"));
8583
subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload"));
86-
assertThat(getEventCountsTracker(), is(Set.of("1", "2")));
84+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2")));
8785

8886
// after 2 minutes
8987
fakeTicker.advance(Duration.ofMinutes(2));
9088
subscriber.onEvent(new GHSubscriberEvent("3", "origin", GHEvent.PUSH, "payload"));
9189
subscriber.onEvent(new GHSubscriberEvent("4", "origin", GHEvent.PUSH, "payload"));
92-
assertThat(getEventCountsTracker(), is(Set.of("1", "2", "3", "4")));
93-
assertThat(getEventCountsTracker().size(), is(4));
90+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2", "3", "4")));
91+
assertThat(subscriber.getEventCountsTracker().size(), is(4));
9492

9593
// 10 minutes 1 second later
9694
fakeTicker.advance(Duration.ofMinutes(8).plusSeconds(1));
97-
assertThat(getEventCountsTracker(), is(Set.of("3", "4")));
98-
assertThat(getEventCountsTracker().size(), is(2));
95+
assertThat(subscriber.getEventCountsTracker(), is(Set.of("3", "4")));
96+
assertThat(subscriber.getEventCountsTracker().size(), is(2));
9997
}
10098

10199
private static class FakeTicker implements Ticker {

0 commit comments

Comments
 (0)