Skip to content

Fix viewer lookup, cache notice parts. #120

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

Open
vLuckyyy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix viewer lookup, cache notice parts. #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions multification-core/src/com/eternalcode/multification/notice/Notice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,17 @@
public class Notice {

private final Map<NoticeKey<?>, NoticePart<?>> parts = new LinkedHashMap<>();
private List<NoticePart<?>> cachedParts;

protected Notice(Map<NoticeKey<?>, NoticePart<?>> parts) {
this.parts.putAll(parts);
}

public List<NoticePart<?>> parts() {
return this.parts.values().stream()
.toList();
if (cachedParts == null) {
cachedParts = List.copyOf(this.parts.values());
}
Comment on lines +36 to +38
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 11, 2026

Choose a reason for hiding this comment

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

medium

The lazy initialization of cachedParts is not thread-safe. If multiple threads call parts() concurrently when cachedParts is null, they might all enter this if block, leading to redundant computation. While this is a benign race condition in this specific case, it's better to use a standard thread-safe lazy initialization pattern.

One option is to use double-checked locking, which would require making cachedParts volatile.

private volatile List<NoticePart<?>> cachedParts;
//...
public List<NoticePart<?>> parts() {
    if (cachedParts == null) {
        synchronized (this) {
            if (cachedParts == null) {
                cachedParts = List.copyOf(this.parts.values());
            }
        }
    }
    return cachedParts;
}

Alternatively, a simpler and safer approach would be to initialize cachedParts in the constructor and make it final, since the parts map is effectively immutable after construction.

return cachedParts;
}

public static <T extends NoticeContent> Notice of(NoticeKey<T> key, T content) {
Expand Down Expand Up @@ -261,7 +264,8 @@ public B bossBar(BossBar.Color color, BossBar.Overlay overlay, Duration duration
}

public B bossBar(BossBar.Color color, Duration duration, String message) {
return this.withPart(NoticeKey.BOSS_BAR, new BossBarContent(color, Optional.empty(), duration, OptionalDouble.empty(), message));
return this.withPart(NoticeKey.BOSS_BAR,
new BossBarContent(color, Optional.empty(), duration, OptionalDouble.empty(), message));
}

}
Expand Down
32 changes: 23 additions & 9 deletions multification-core/src/com/eternalcode/multification/notice/NoticeBroadcastImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -78,13 +77,10 @@ public B player(UUID player) {
@Override
@CheckReturnValue
public B players(Iterable<UUID> players) {
Set<VIEWER> viewers = new HashSet<>();

for (UUID player : players) {
viewers.add(this.viewerProvider.player(player));
this.viewers.add(this.viewerProvider.player(player));
}

this.viewers.addAll(viewers);
return this.getThis();
}

Expand Down Expand Up @@ -258,11 +254,10 @@ private void sendTranslatedMessages(LanguageViewersIndex<VIEWER> viewersIndex, T
continue;
}

Set<VIEWER> languageViewers = viewersIndex.getViewers(language);
TranslatedFormatter translatedFormatter = this.prepareFormatterForLanguage(language);

for (Notice notice : notificationsForLang) {
Set<VIEWER> languageViewers = viewersIndex.getViewers(language);

for (VIEWER viewer : languageViewers) {
Audience audience = audienceConverter.convert(viewer);

Expand Down Expand Up @@ -298,23 +293,42 @@ private TranslatedNoticesIndex prepareTranslatedNotices(Set<Locale> languages) {
});
}

private final Map<Locale, TranslatedFormatter> formatterCache = new HashMap<>();
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 11, 2026

Choose a reason for hiding this comment

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

high

The formatterCache is initialized as a HashMap, which is not thread-safe. Since send() can be executed asynchronously via sendAsync(), the prepareFormatterForLanguage method can be called by multiple threads concurrently. This can lead to race conditions when accessing formatterCache, potentially causing data corruption.

To ensure thread safety, you should use a thread-safe map implementation like java.util.concurrent.ConcurrentHashMap.

Suggested change
private final Map<Locale, TranslatedFormatter> formatterCache = new HashMap<>();
private final Map<Locale, TranslatedFormatter> formatterCache = new java.util.concurrent.ConcurrentHashMap<>();


protected TranslatedFormatter prepareFormatterForLanguage(Locale language) {
TranslatedFormatter cached = formatterCache.get(language);
if (cached != null && !cached.isDirty()) {
return cached;
}

TRANSLATION translation = this.translationProvider.provide(language);
Formatter translatedFormatter = new Formatter();

for (Map.Entry<String, TextMessageProvider<TRANSLATION>> entry : this.placeholders.entrySet()) {
translatedFormatter.register(entry.getKey(), () -> entry.getValue().extract(translation));
String value = entry.getValue().extract(translation);
translatedFormatter.register(entry.getKey(), value);
}

return new TranslatedFormatter(translatedFormatter);
TranslatedFormatter formatter = new TranslatedFormatter(translatedFormatter);
formatterCache.put(language, formatter);
return formatter;
}

protected class TranslatedFormatter {

protected final Formatter translatedPlaceholders;
private final int expectedFormatterCount;
private final int expectedPlaceholderCount;

protected TranslatedFormatter(Formatter translatedPlaceholders) {
this.translatedPlaceholders = translatedPlaceholders;
this.expectedFormatterCount = NoticeBroadcastImpl.this.formatters.size();
this.expectedPlaceholderCount = NoticeBroadcastImpl.this.placeholders.size();
}

protected boolean isDirty() {
return NoticeBroadcastImpl.this.formatters.size() != expectedFormatterCount
|| NoticeBroadcastImpl.this.placeholders.size() != expectedPlaceholderCount;
}
Comment on lines +329 to 332
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 11, 2026

Choose a reason for hiding this comment

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

high

The isDirty check is flawed because it only checks the size of the formatters and placeholders collections. This logic fails to detect modifications that don't change the collection size, such as replacing a placeholder with a new value for an existing key (e.g., calling placeholder("{key}", "new_value") after placeholder("{key}", "old_value") was already called). This can lead to stale formatters being served from the cache, resulting in incorrect messages.

A more robust approach would be to use a modification counter (e.g., an int or AtomicInteger field in NoticeBroadcastImpl) that is incremented every time formatters or placeholders are modified. The TranslatedFormatter would then store the modCount at creation time and compare it with the current modCount in isDirty().


public String format(String text, VIEWER viewer) {
Expand Down
3 changes: 1 addition & 2 deletions ...cation-core/src/com/eternalcode/multification/notice/resolver/NoticeResolverRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ public Optional<NoticeDeserializeResult<?>> deserialize(String key, NoticeSerdes
if (deserializeResult != null) {
return Optional.of(deserializeResult);
}
}
catch (Throwable exception) {
} catch (Exception exception) {
illegalArgumentException.addSuppressed(exception);
}
}
Expand Down