From f016d03511f76ee1a9249bf4a6a11a1fce5ab0a2 Mon Sep 17 00:00:00 2001 From: guruprasad Date: Tue, 25 Feb 2025 06:34:04 +0530 Subject: [PATCH 1/2] Do not use static state variables in Extension class --- .../admin/GitHubDuplicateEventsMonitor.java | 39 +++++------ .../GitHubDuplicateEventsMonitorUnitTest.java | 68 +++++++++---------- 2 files changed, 53 insertions(+), 54 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java index e24713cf6..c56ec1a40 100644 --- a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java +++ b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java @@ -12,6 +12,7 @@ import com.google.common.annotations.VisibleForTesting; import hudson.Extension; +import hudson.ExtensionList; import hudson.model.AdministrativeMonitor; import hudson.model.Item; import jenkins.model.Jenkins; @@ -57,7 +58,7 @@ String getLastDuplicateUrl() { @Override public boolean isActivated() { - return DuplicateEventsSubscriber.isDuplicateEventSeen(); + return ExtensionList.lookupSingleton(DuplicateEventsSubscriber.class).isDuplicateEventSeen(); } @Override @@ -75,7 +76,7 @@ public void checkRequiredPermission() { public HttpResponse doGetLastDuplicatePayload() { Jenkins.get().checkPermission(Jenkins.SYSTEM_READ); JSONObject data; - var lastDuplicate = DuplicateEventsSubscriber.getLastDuplicate(); + var lastDuplicate = ExtensionList.lookupSingleton(DuplicateEventsSubscriber.class).getLastDuplicate(); if (lastDuplicate != null) { data = JSONObject.fromObject(lastDuplicate.ghSubscriberEvent().getPayload()); } else { @@ -102,7 +103,7 @@ public static final class DuplicateEventsSubscriber extends GHEventsSubscriber { private static final Logger LOGGER = Logger.getLogger(DuplicateEventsSubscriber.class.getName()); - private static Ticker ticker = Ticker.systemTicker(); + private Ticker ticker = Ticker.systemTicker(); /** * Caches GitHub event GUIDs for 10 minutes to track recent events to detect duplicates. *

@@ -114,21 +115,21 @@ public static final class DuplicateEventsSubscriber extends GHEventsSubscriber { * timestamp (assuming caffeine internally keeps long) takes 8 bytes; total of 44 bytes * per entry. So the maximum memory consumed by this cache is 24k * 44 = 1056k = 1.056 MB. */ - private static final Cache EVENT_TRACKER = Caffeine.newBuilder() - .maximumSize(24_000L) - .expireAfterWrite(Duration.ofMinutes(10)) - .ticker(() -> ticker.read()) - .build(); + private final Cache eventTracker = Caffeine.newBuilder() + .maximumSize(24_000L) + .expireAfterWrite(Duration.ofMinutes(10)) + .ticker(() -> ticker.read()) + .build(); private static final Object DUMMY = new Object(); - private static volatile TrackedDuplicateEvent lastDuplicate; + private volatile TrackedDuplicateEvent lastDuplicate; public record TrackedDuplicateEvent( String eventGuid, Instant lastUpdated, GHSubscriberEvent ghSubscriberEvent) { } private static final Duration TWENTY_FOUR_HOURS = Duration.ofHours(24); @VisibleForTesting @Restricted(NoExternalUse.class) - static void setTicker(Ticker testTicker) { + void setTicker(Ticker testTicker) { ticker = testTicker; } @@ -174,10 +175,10 @@ protected void onEvent(final GHSubscriberEvent event) { if (eventGuid == null) { return; } - if (EVENT_TRACKER.getIfPresent(eventGuid) != null) { + if (eventTracker.getIfPresent(eventGuid) != null) { lastDuplicate = new TrackedDuplicateEvent(eventGuid, getNow(), event); } - EVENT_TRACKER.put(eventGuid, DUMMY); + eventTracker.put(eventGuid, DUMMY); } /** @@ -187,16 +188,16 @@ protected void onEvent(final GHSubscriberEvent event) { * * @return {@code true} if a duplicate was seen in the last 24 hours, {@code false} otherwise. */ - public static boolean isDuplicateEventSeen() { + public boolean isDuplicateEventSeen() { return lastDuplicate != null && Duration.between(lastDuplicate.lastUpdated(), getNow()).compareTo(TWENTY_FOUR_HOURS) < 0; } - private static Instant getNow() { + private Instant getNow() { return Instant.ofEpochSecond(0L, ticker.read()); } - public static TrackedDuplicateEvent getLastDuplicate() { + public TrackedDuplicateEvent getLastDuplicate() { return lastDuplicate; } @@ -206,10 +207,10 @@ public static TrackedDuplicateEvent getLastDuplicate() { */ @VisibleForTesting @Restricted(NoExternalUse.class) - static Set getEventCountsTracker() { - return EVENT_TRACKER.asMap().keySet().stream() - .filter(key -> EVENT_TRACKER.getIfPresent(key) != null) - .collect(Collectors.toSet()); + Set getEventCountsTracker() { + return eventTracker.asMap().keySet().stream() + .filter(key -> eventTracker.getIfPresent(key) != null) + .collect(Collectors.toSet()); } } } diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java index 7f92fd1f8..95919417b 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java @@ -3,9 +3,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; -import static org.jenkinsci.plugins.github.admin.GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.getEventCountsTracker; -import static org.jenkinsci.plugins.github.admin.GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.getLastDuplicate; -import static org.jenkinsci.plugins.github.admin.GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.isDuplicateEventSeen; import java.time.Duration; import java.time.Instant; @@ -21,81 +18,82 @@ @For(GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.class) public class GitHubDuplicateEventsMonitorUnitTest { - private final GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber subscriber - = new GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber(); - @Test public void onEventShouldTrackEventAndKeepTrackOfLastDuplicate() { + var subscriber = new GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber(); + var now = Instant.parse("2025-02-05T03:00:00Z"); var after1Sec = Instant.parse("2025-02-05T03:00:01Z"); var after2Sec = Instant.parse("2025-02-05T03:00:02Z"); FakeTicker fakeTicker = new FakeTicker(now); - GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.setTicker(fakeTicker); + subscriber.setTicker(fakeTicker); - assertThat("lastDuplicate is null at first", getLastDuplicate(), is(nullValue())); - assertThat("should not throw NPE", isDuplicateEventSeen(), is(false)); + assertThat("lastDuplicate is null at first", subscriber.getLastDuplicate(), is(nullValue())); + assertThat("should not throw NPE", subscriber.isDuplicateEventSeen(), is(false)); // send a null event subscriber.onEvent(new GHSubscriberEvent(null, "origin", GHEvent.PUSH, "payload")); - assertThat("null event is not tracked", getEventCountsTracker().size(), is(0)); - assertThat("lastDuplicate is still null", getLastDuplicate(), is(nullValue())); + assertThat("null event is not tracked", subscriber.getEventCountsTracker().size(), is(0)); + assertThat("lastDuplicate is still null", subscriber.getLastDuplicate(), is(nullValue())); // at present subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload")); - assertThat(getEventCountsTracker(), is(Set.of("1"))); - assertThat(getLastDuplicate(), is(nullValue())); - assertThat(isDuplicateEventSeen(), is(false)); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1"))); + assertThat(subscriber.getLastDuplicate(), is(nullValue())); + assertThat(subscriber.isDuplicateEventSeen(), is(false)); subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload")); - assertThat(getLastDuplicate(), is(nullValue())); - assertThat(isDuplicateEventSeen(), is(false)); - assertThat(getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getLastDuplicate(), is(nullValue())); + assertThat(subscriber.isDuplicateEventSeen(), is(false)); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); subscriber.onEvent(new GHSubscriberEvent(null, "origin", GHEvent.PUSH, "payload")); - assertThat(getLastDuplicate(), is(nullValue())); - assertThat(getEventCountsTracker(), is(Set.of("1", "2"))); - assertThat(isDuplicateEventSeen(), is(false)); + assertThat(subscriber.getLastDuplicate(), is(nullValue())); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.isDuplicateEventSeen(), is(false)); // after a second fakeTicker.advance(Duration.ofSeconds(1)); subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload")); - assertThat(getLastDuplicate().eventGuid(), is("1")); - assertThat(getLastDuplicate().lastUpdated(), is(after1Sec)); - assertThat(getEventCountsTracker(), is(Set.of("1", "2"))); - assertThat(isDuplicateEventSeen(), is(true)); + assertThat(subscriber.getLastDuplicate().eventGuid(), is("1")); + assertThat(subscriber.getLastDuplicate().lastUpdated(), is(after1Sec)); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.isDuplicateEventSeen(), is(true)); // second occurrence for another event after 2 seconds fakeTicker.advance(Duration.ofSeconds(1)); subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload")); - assertThat(getLastDuplicate().eventGuid(), is("2")); - assertThat(getLastDuplicate().lastUpdated(), is(after2Sec)); - assertThat(getEventCountsTracker(), is(Set.of("1", "2"))); - assertThat(isDuplicateEventSeen(), is(true)); + assertThat(subscriber.getLastDuplicate().eventGuid(), is("2")); + assertThat(subscriber.getLastDuplicate().lastUpdated(), is(after2Sec)); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.isDuplicateEventSeen(), is(true)); // 24 hours has passed; note we already added 2 seconds/ so effectively 24h 2sec now. fakeTicker.advance(Duration.ofHours(24)); - assertThat(isDuplicateEventSeen(), is(false)); + assertThat(subscriber.isDuplicateEventSeen(), is(false)); } @Test public void checkOldEntriesAreExpiredAfter10Minutes() { + var subscriber = new GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber(); + var now = Instant.parse("2025-02-05T03:00:00Z"); FakeTicker fakeTicker = new FakeTicker(now); - GitHubDuplicateEventsMonitor.DuplicateEventsSubscriber.setTicker(fakeTicker); + subscriber.setTicker(fakeTicker); // at present subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload")); subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload")); - assertThat(getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); // after 2 minutes fakeTicker.advance(Duration.ofMinutes(2)); subscriber.onEvent(new GHSubscriberEvent("3", "origin", GHEvent.PUSH, "payload")); subscriber.onEvent(new GHSubscriberEvent("4", "origin", GHEvent.PUSH, "payload")); - assertThat(getEventCountsTracker(), is(Set.of("1", "2", "3", "4"))); - assertThat(getEventCountsTracker().size(), is(4)); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2", "3", "4"))); + assertThat(subscriber.getEventCountsTracker().size(), is(4)); // 10 minutes 1 second later fakeTicker.advance(Duration.ofMinutes(8).plusSeconds(1)); - assertThat(getEventCountsTracker(), is(Set.of("3", "4"))); - assertThat(getEventCountsTracker().size(), is(2)); + assertThat(subscriber.getEventCountsTracker(), is(Set.of("3", "4"))); + assertThat(subscriber.getEventCountsTracker().size(), is(2)); } private static class FakeTicker implements Ticker { From d86bd7fee428a89e7cf761e91e0af881e76486bb Mon Sep 17 00:00:00 2001 From: guruprasad Date: Wed, 26 Feb 2025 08:22:37 +0530 Subject: [PATCH 2/2] rename the test helper method for better readability --- .../admin/GitHubDuplicateEventsMonitor.java | 2 +- .../GitHubDuplicateEventsMonitorUnitTest.java | 22 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java index c56ec1a40..794f3db04 100644 --- a/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java +++ b/src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java @@ -207,7 +207,7 @@ public TrackedDuplicateEvent getLastDuplicate() { */ @VisibleForTesting @Restricted(NoExternalUse.class) - Set getEventCountsTracker() { + Set getPresentEventKeys() { return eventTracker.asMap().keySet().stream() .filter(key -> eventTracker.getIfPresent(key) != null) .collect(Collectors.toSet()); diff --git a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java index 95919417b..fd8195ac4 100644 --- a/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java @@ -32,21 +32,21 @@ public void onEventShouldTrackEventAndKeepTrackOfLastDuplicate() { assertThat("should not throw NPE", subscriber.isDuplicateEventSeen(), is(false)); // send a null event subscriber.onEvent(new GHSubscriberEvent(null, "origin", GHEvent.PUSH, "payload")); - assertThat("null event is not tracked", subscriber.getEventCountsTracker().size(), is(0)); + assertThat("null event is not tracked", subscriber.getPresentEventKeys().size(), is(0)); assertThat("lastDuplicate is still null", subscriber.getLastDuplicate(), is(nullValue())); // at present subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload")); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1"))); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1"))); assertThat(subscriber.getLastDuplicate(), is(nullValue())); assertThat(subscriber.isDuplicateEventSeen(), is(false)); subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload")); assertThat(subscriber.getLastDuplicate(), is(nullValue())); assertThat(subscriber.isDuplicateEventSeen(), is(false)); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1", "2"))); subscriber.onEvent(new GHSubscriberEvent(null, "origin", GHEvent.PUSH, "payload")); assertThat(subscriber.getLastDuplicate(), is(nullValue())); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1", "2"))); assertThat(subscriber.isDuplicateEventSeen(), is(false)); // after a second @@ -54,7 +54,7 @@ public void onEventShouldTrackEventAndKeepTrackOfLastDuplicate() { subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload")); assertThat(subscriber.getLastDuplicate().eventGuid(), is("1")); assertThat(subscriber.getLastDuplicate().lastUpdated(), is(after1Sec)); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1", "2"))); assertThat(subscriber.isDuplicateEventSeen(), is(true)); // second occurrence for another event after 2 seconds @@ -62,7 +62,7 @@ public void onEventShouldTrackEventAndKeepTrackOfLastDuplicate() { subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload")); assertThat(subscriber.getLastDuplicate().eventGuid(), is("2")); assertThat(subscriber.getLastDuplicate().lastUpdated(), is(after2Sec)); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1", "2"))); assertThat(subscriber.isDuplicateEventSeen(), is(true)); // 24 hours has passed; note we already added 2 seconds/ so effectively 24h 2sec now. @@ -81,19 +81,19 @@ public void checkOldEntriesAreExpiredAfter10Minutes() { // at present subscriber.onEvent(new GHSubscriberEvent("1", "origin", GHEvent.PUSH, "payload")); subscriber.onEvent(new GHSubscriberEvent("2", "origin", GHEvent.PUSH, "payload")); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2"))); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1", "2"))); // after 2 minutes fakeTicker.advance(Duration.ofMinutes(2)); subscriber.onEvent(new GHSubscriberEvent("3", "origin", GHEvent.PUSH, "payload")); subscriber.onEvent(new GHSubscriberEvent("4", "origin", GHEvent.PUSH, "payload")); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("1", "2", "3", "4"))); - assertThat(subscriber.getEventCountsTracker().size(), is(4)); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("1", "2", "3", "4"))); + assertThat(subscriber.getPresentEventKeys().size(), is(4)); // 10 minutes 1 second later fakeTicker.advance(Duration.ofMinutes(8).plusSeconds(1)); - assertThat(subscriber.getEventCountsTracker(), is(Set.of("3", "4"))); - assertThat(subscriber.getEventCountsTracker().size(), is(2)); + assertThat(subscriber.getPresentEventKeys(), is(Set.of("3", "4"))); + assertThat(subscriber.getPresentEventKeys().size(), is(2)); } private static class FakeTicker implements Ticker {