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.

Sign up for GitHub

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

Do not use static state variables in Extension class #389

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not use static state variables in Extension class #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f016d03
Do not use static state variables in Extension class
gbhat618 Feb 25, 2025
d86bd7f
rename the test helper method for better readability
gbhat618 Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 20 additions & 19 deletions src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -57,7 +58,7 @@

@Override
public boolean isActivated() {
return DuplicateEventsSubscriber.isDuplicateEventSeen();
return ExtensionList.lookupSingleton(DuplicateEventsSubscriber.class).isDuplicateEventSeen();
}

@Override
Expand All @@ -75,7 +76,7 @@
public HttpResponse doGetLastDuplicatePayload() {
Jenkins.get().checkPermission(Jenkins.SYSTEM_READ);
JSONObject data;
var lastDuplicate = DuplicateEventsSubscriber.getLastDuplicate();
var lastDuplicate = ExtensionList.lookupSingleton(DuplicateEventsSubscriber.class).getLastDuplicate();
Copy link
Contributor Author

@gbhat618 gbhat618 Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using ExtensionList.lookupSingleton in two places in this class.

I considered keeping a class-level subscriber field, but that seem to require extra code to manage the reference correctly.

  • Since both this monitor class and the subscriber have @Extension, I think they can't store direct references to each other—perhaps because their initialization order isn't guaranteed?
  • We could store the reference during execution (if (subscriber != null) { subscriber = ExtensionList.lookupSingleton(...); }), but extensions are already looked up in more hot code path - ref code (executed for every event)

So I think this approach is fine - (ExtensionList.lookupSingleton probably doesn't introduce any overhead)

if (lastDuplicate != null) {
data = JSONObject.fromObject(lastDuplicate.ghSubscriberEvent().getPayload());
} else {
Expand All @@ -102,7 +103,7 @@

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.
* <p>
Expand All @@ -114,21 +115,21 @@
* 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<String, Object> EVENT_TRACKER = Caffeine.newBuilder()
.maximumSize(24_000L)
.expireAfterWrite(Duration.ofMinutes(10))
.ticker(() -> ticker.read())
.build();
private final Cache<String, Object> 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;
}

Expand Down Expand Up @@ -174,10 +175,10 @@
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);
}

/**
Expand All @@ -187,16 +188,16 @@
*
* @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;
}

Expand All @@ -206,10 +207,10 @@
*/
@VisibleForTesting
@Restricted(NoExternalUse.class)
static Set<String> getEventCountsTracker() {
return EVENT_TRACKER.asMap().keySet().stream()
.filter(key -> EVENT_TRACKER.getIfPresent(key) != null)
.collect(Collectors.toSet());
Set<String> getPresentEventKeys() {
return eventTracker.asMap().keySet().stream()
.filter(key -> eventTracker.getIfPresent(key) != null)

Check warning on line 212 in src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 212 is only partially covered, one branch is missing
Comment on lines +211 to +212
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I think this would be better written using a stream on entrySet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entrySet because it is more performance optimal?

would be like,

return eventTracker.asMap().entrySet().stream()
                   .filter(entry -> entry.getValue() != null)
                   .map(Map.Entry::getKey)
                   .collect(Collectors.toSet());

But since this is a helper method for tests only, and we only need the keys, ig probably keeping on keySet() is easy read.. 🤔

(I now renamed the method to getPresentEventKeys which makes more sense)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it is just a bad pattern to iterate a map’s key set and then look up the value each time. Does not really matter of course.

.collect(Collectors.toSet());
}
}
}
68 changes: 33 additions & 35 deletions src/test/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitorUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.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(getEventCountsTracker(), is(Set.of("1")));
assertThat(getLastDuplicate(), is(nullValue()));
assertThat(isDuplicateEventSeen(), is(false));
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(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.getPresentEventKeys(), 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.getPresentEventKeys(), 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.getPresentEventKeys(), 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.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.
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.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(getEventCountsTracker(), is(Set.of("1", "2", "3", "4")));
assertThat(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(getEventCountsTracker(), is(Set.of("3", "4")));
assertThat(getEventCountsTracker().size(), is(2));
assertThat(subscriber.getPresentEventKeys(), is(Set.of("3", "4")));
assertThat(subscriber.getPresentEventKeys().size(), is(2));
}

private static class FakeTicker implements Ticker {
Expand Down