Full rework ice adapter #83
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the ICE adapter from a monolithic architecture to a modular, service-oriented design. Major changes include: introducing UI layers with JavaFX windows and controllers, replacing static debug components with instance-based services, restructuring peer management into a plugin-based module system, establishing abstraction boundaries via service interfaces, and updating build configurations to support the new architecture across multiple submodules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant IA as IceAdapter
participant GS as GameSession
participant CS as ConnectService
participant Peer as Peer
participant Mod as PeerModule<br/>(EventBus, FA, ICE)
participant ICE as ICE4J Agent
Client->>IA: connectToPeer(login, id, offer, combo)
IA->>GS: createGameSession()
GS->>Peer: new Peer(id, login)
Peer->>Peer: init()
Peer->>Mod: initModules()
loop Per PeerModule
Mod->>Mod: createModule(Peer)
Mod->>Peer: register listener
end
Peer->>Peer: startModules()
GS->>CS: onChangeIceState(Peer, NEW, GATHERING)
CS->>ICE: createAgent()
CS->>ICE: gatherCandidates()
Note over ICE: STUN/TURN harvest
CS->>CS: awaiting candidates (timeout 5s)
alt RPC receives remote candidates
Client->>GS: onIceMessageFromRPC(candidates)
GS->>CS: onMessageFromRPC(Peer, message)
CS->>ICE: unpackCandidates()
CS->>CS: onChangeIceState(CHECKING)
end
CS->>CS: checking(Peer)
ICE->>ICE: connectivity checks
ICE->>ICE: pair selected
CS->>CS: onChangeIceState(CONNECTED)
CS->>Peer: onConnected()
Peer->>Mod: fire onConnected event
Mod->>Mod: start modules
Peer->>Peer: ready for data forwarding
sequenceDiagram
participant FA as Forwarder (FA)
participant P2P as PeerToPeerListener
participant Peer as Peer
participant F2P as FaToPeerModule
participant ICE as ICE Component
participant P2F as PeerToFaModule
FA->>P2P: UDP datagram (peer data)
P2P->>Peer: onSendToPeer(data)
Peer->>F2P: trigger event
F2P->>ICE: send via component
ICE->>P2P: receive data
P2P->>P2P: parse COMMAND_FA prefix
P2P->>Peer: onSendToFaSocket(data)
Peer->>P2F: trigger event
P2F->>FA: DatagramSocket.send(localhost:port)
Note over P2P,P2F: Echo-based connectivity<br/>checking also wired<br/>via same path
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes The changes constitute a fundamental architectural refactoring affecting core subsystems (peer lifecycle, ICE connection handling, debug/UI framework), introducing new service abstractions, modular peer plugin system, and UI layer. The diff spans 60+ files with diverse patterns: new class introductions, multi-file deletions, signature changes across service boundaries, and intricate state machine rewiring in ICE connection flows. High logic density in ConnectServiceCommon, GameSession, and Peer classes, plus heterogeneous file types (Java classes, FXML, Gradle configs, resources) demand careful reasoning per file and cross-file dependency validation. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ice-adapter/src/main/java/com/faforever/iceadapter/gpgnet/FaDataInputStream.java (1)
85-99: Add upper bound validation for string size.The method validates that
sizeis non-negative (lines 88-90) but doesn't enforce an upper limit. A malicious or corrupted stream could specify an extremely large size, causing memory exhaustion when allocating the byte array (line 96).🔎 Proposed fix
Add a constant and validation:
public class FaDataInputStream extends InputStream { private static final int MAX_CHUNK_SIZE = 10; + private static final int MAX_STRING_SIZE = 1048576; // 1MB private static final int FIELD_TYPE_INT = 0; private static final int FIELD_TYPE_STRING = 1;public String readString() throws IOException { int size = readInt(); if (size < 0) { throw new IOException("Invalid string length: " + size); } + if (size > MAX_STRING_SIZE) { + throw new IOException("String too large: " + size); + } + if (size == 0) { return ""; }ice-adapter/src/main/java/com/faforever/iceadapter/util/PingWrapper.java (1)
40-61: Blocking I/O on ForkJoinPool.commonPool() can cause thread starvation.The
supplyAsync()call without an explicit executor usesForkJoinPool.commonPool(), which is optimized for CPU-bound tasks. Theprocess.waitFor()on line 43 is a blocking I/O operation that will occupy a thread from the common pool, potentially starving other tasks.🔎 Proposed fix to use a dedicated executor
Add a dedicated executor for I/O operations:
+import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + @Slf4j public class PingWrapper { + private static final ExecutorService IO_EXECUTOR = Executors.newCachedThreadPool( + r -> { + Thread t = new Thread(r, "ping-worker"); + t.setDaemon(true); + return t; + } + ); + static final Pattern WINDOWS_OUTPUT_PATTERN = Pattern.compile("Average = (\\d+)ms", Pattern.MULTILINE);Then use it:
- return CompletableFuture.supplyAsync( + return CompletableFuture.supplyAsync( () -> { try { process.waitFor(); InputStreamReader reader = new InputStreamReader(process.getInputStream()); String output = CharStreams.toString(reader); reader.close(); Matcher m = output_pattern.matcher(output); if (m.find()) { double result = Double.parseDouble(m.group(1)); log.debug("Pinged {} with an RTT of {}", address, result); return result; } else { log.warn("Failed to ping {}: output='{}'", address, output); throw new RuntimeException("Failed to contact the host or parse ping output"); } } catch (InterruptedException | IOException | RuntimeException e) { throw new CompletionException(e); } - }); + }, IO_EXECUTOR);ice-adapter/src/main/java/com/faforever/iceadapter/rpc/RPCService.java (1)
55-59: Potential NPE onInfoWindow.INSTANCE.showWindow().After
Debug.init(),InfoWindow.INSTANCEmight still be null if initialization fails. The code assumesINSTANCEis set after theifblock.🔎 Proposed fix
if (InfoWindow.INSTANCE == null) { Debug.ENABLE_INFO_WINDOW = true; Debug.init(); } - InfoWindow.INSTANCE.showWindow(); + if (InfoWindow.INSTANCE != null) { + InfoWindow.INSTANCE.showWindow(); + }
🟠 Major comments (20)
client/src/main/java/client/TestClient.java-83-83 (1)
83-83: Fix the GDPR dialog logic: the default skip renders the feature unreachable.Changing
skipGDRPtotrueby default makes the GDPR dialog impossible to display, since line 119-121 only shows it whenskipGDRPisfalse. This breaks the ability to test or review the GDPR consent flow. Additionally:
- The
--skipargument (lines 99-101) is now redundant: it setsskipGDRP=truewhen it's alreadytrueby default.- There is no command-line option to show the GDPR dialog (e.g.,
--show-gdpr).- Typos throughout: "GDRP" (lines 92) should be "GDPR"; variable name
skipGDRPshould beskipGDPR.Either change the default to
skipGDRP = falseto make the dialog show by default (with--skipto bypass it), or remove the GDPR dialog feature entirely if it's not needed. Also fix the typos.ice-adapter/src/main/java/com/faforever/iceadapter/util/DatagramSocketUtils.java-14-23 (1)
14-23: Socket buffer size is dangerously small and will cause packet loss.The method sets both receive and send socket buffers to
MAX_SIZE_PACKET(1472 bytes), which is the maximum UDP packet payload size. However, socket buffers should be much larger—typically 64KB to 256KB or more—to allow the OS to queue multiple packets while the application processes them. Setting buffers to just 1472 bytes means only a single packet can be buffered, causing packet drops under any moderate load.The confusion appears to be between:
- Packet size (MAX_SIZE_PACKET = 1472 bytes) ✓ correct for individual datagram buffers
- Socket buffer size (should be 64KB+) ✗ currently set to 1472 bytes
🔎 Recommended fix: Use appropriate socket buffer sizes
public void resizeBuffer(DatagramSocket socket) { try { - socket.setReceiveBufferSize(MAX_SIZE_PACKET); - socket.setSendBufferSize(MAX_SIZE_PACKET); + // Set socket buffers to reasonable sizes for network traffic + // Typical values: 64KB-256KB depending on expected load + socket.setReceiveBufferSize(256 * 1024); // 256 KB + socket.setSendBufferSize(256 * 1024); // 256 KB } catch (SocketException e) { if (!socket.isClosed()) { - log.error("Failed to resize socket buffer on {}", MAX_SIZE_PACKET, e); + log.error("Failed to resize socket buffer", e); } } }Alternatively, if you want to keep the buffer size configurable, define separate constants:
public static final int SOCKET_BUFFER_SIZE = 256 * 1024; // 256 KBice-adapter/src/main/java/com/faforever/iceadapter/ice/PeerConnectionSuccessMonitor.java-93-98 (1)
93-98: Missing listener cleanup in shutdown().The
shutdown()method terminates the scheduler and cancels the timeout task, but does not unregister the listeners added instart()(lines 48-49). This means the agent and mediaStream will retain references to this monitor even after shutdown, preventing garbage collection.This issue is addressed by the
cleanup()method suggested in the earlier comment forstart().🔎 Proposed fix
public void shutdown() { + cleanup(); // Unregister listeners and cancel timeout scheduler.shutdown(); - if (scheduledFuture != null) { - scheduledFuture.cancel(true); - } }Where
cleanup()is as defined in the earlier suggestion.Committable suggestion skipped: line range outside the PR's diff.
ice-adapter/src/main/java/com/faforever/iceadapter/ice/PeerConnectionSuccessMonitor.java-17-23 (1)
17-23: Resource leak: scheduler created eagerly at field initialization.The
ScheduledExecutorServiceis created when the instance is constructed (line 18), which means:
- If
start()is never called, the scheduler thread still exists untilshutdown()is called- If
shutdown()is not called (e.g., due to an exception or oversight), the scheduler thread leaksAdditionally,
Executors.newScheduledThreadPool(1)is less explicit thanExecutors.newSingleThreadScheduledExecutor()for a single-threaded scheduler.🔎 Proposed fix: lazy initialization
- private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1); + private ScheduledExecutorService scheduler; private ScheduledFuture<?> scheduledFuture; private CompletableFuture<Boolean> future; private String name = ""; private final long timeoutMs; private final Peer peer; @Override public CompletableFuture<Boolean> start() { future = new CompletableFuture<>(); + scheduler = Executors.newSingleThreadScheduledExecutor();Alternatively, consider using a shared scheduler passed via constructor or using the peer's executor service if available.
Committable suggestion skipped: line range outside the PR's diff.
ice-adapter/src/main/java/com/faforever/iceadapter/gpgnet/FaDataOutputStream.java-30-30 (1)
30-30: Fix inconsistent closed-state behavior and race conditions.The implementation has several issues:
Inconsistent behavior:
flush()silently returns when closed (lines 77-79), whilewrite()(line 41) andwriteMessage()(line 65) throwIOException. This inconsistency can mask errors.Race condition in write(): Line 41 checks
closedbefore acquiring the lock. Another thread could close the stream between the check and lock acquisition, leading to operations on a closed stream.Insufficient synchronization: The
volatilekeyword onclosed(line 30) ensures visibility but doesn't prevent the check-then-act race inwrite()and the lock/unlock window in other methods.🔎 Proposed fix
@Override public void write(int b) throws IOException { - if (closed) throw new IOException("Stream is closed"); writer.lock(); try { + if (closed) throw new IOException("Stream is closed"); outputStream.write(b); } finally { writer.unlock(); } } @Override public void flush() throws IOException { - if (closed) { - return; - } writer.lock(); try { + if (closed) throw new IOException("Stream is closed"); outputStream.flush(); } finally { writer.unlock(); } }Also applies to: 40-48, 58-72, 76-86, 89-98
ice-adapter/src/main/java/com/faforever/iceadapter/UiStarter.java-1-22 (1)
1-22: Document purpose and add initialization verification.This class appears to be a test/development harness but has several issues:
Missing documentation: No class-level Javadoc or comments explaining this is for testing/development purposes.
Race condition risk: Lines 19-20 assume
IceAdapter.INSTANCEis initialized synchronously byIceAdapter.main(). If initialization is asynchronous or deferred, this will result in NPE.No state verification:
onJoinGame()is called immediately without verifying the adapter is ready to accept game events.Unclear location: This appears to be test/development code but is located in
src/main/javainstead ofsrc/test/java.🔎 Suggested improvements
package com.faforever.iceadapter; +/** + * Test/development harness for launching the IceAdapter with a UI. + * This class is for development purposes only and should not be used in production. + */ public class UiStarter { public static void main(String[] args) { String[] stubArgs = { "--id=12345", "--game-id=67890", "--login=testUser", "--gpgnet-port=5000", "--rpc-port=5001", "--lobby-port=5002", "--debug-window=true", "--info-window=true" }; IceAdapter.main(stubArgs); IceAdapter adapter = IceAdapter.INSTANCE; + if (adapter == null) { + throw new IllegalStateException("IceAdapter not initialized"); + } + + // Wait for adapter to be fully initialized + // TODO: Add proper readiness check adapter.onJoinGame("Player2", 123); } }Consider moving to
src/test/javaif this is purely for testing.ice-adapter/src/main/java/com/faforever/iceadapter/ice/ModuleBase.java-3-21 (1)
3-21: Reconsider nullable Boolean return types.The
isRunning()andisEnabled()methods returnBoolean(wrapper type) with a default ofnull(lines 14-20). This design is problematic:
- NPE risk: Callers using these methods in conditionals or auto-unboxing contexts will encounter NPE if they don't explicitly check for null.
- Unclear semantics: What does
nullmean? "Not applicable", "unknown", or "not implemented"? Without documentation, this is ambiguous.- Inconsistent with naming: Methods prefixed with
is*typically return boolean primitives, not nullable wrappers.Consider alternatives:
- Use
booleanprimitives with meaningful defaults (e.g.,false).- Use
Optional<Boolean>to make the tri-state explicit.- Add comprehensive Javadoc explaining the null semantics.
🔎 Proposed fix option 1 (boolean primitive)
- default Boolean isRunning() { - return null; + default boolean isRunning() { + return false; } - default Boolean isEnabled() { - return null; + default boolean isEnabled() { + return true; }🔎 Proposed fix option 2 (Optional for explicit tri-state)
+import java.util.Optional; + public interface ModuleBase { default void start() { } default void stop() { } default void init() { } - default Boolean isRunning() { - return null; + default Optional<Boolean> isRunning() { + return Optional.empty(); } - default Boolean isEnabled() { - return null; + default Optional<Boolean> isEnabled() { + return Optional.empty(); } }ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/fa/FaToPeerModule.java-24-24 (1)
24-24:ExecutorServiceis never shut down, causing resource leak.The
executorfield is created as a newVirtualThreadPerTaskExecutorbutstop()never callsexecutor.shutdown(). This will prevent clean shutdown of the executor.🔎 Proposed fix
public void stop() { LockUtil.executeWithLock(peer.getLock(LOCK_MODULE), this::stopListeners); + executor.shutdown(); } private void stopListeners() { - if (peer.isClosing() && listener != null && !listener.isDone()) { + if (listener != null && !listener.isDone()) { listener.cancel(true); listener = null; } }Also applies to: 42-50
ice-adapter/src/main/java/com/faforever/iceadapter/ice/IceServerChecker.java-33-40 (1)
33-40: Uncaught exceptions will silently stop the scheduled task.If
checkerThread()throws any exception,ScheduledExecutorServicewill suppress it and stop future executions without notification. Wrap the body in try-catch to log errors and allow continued execution.🔎 Proposed fix
private void checkerThread() { + try { gameSession.getIceServers().stream() .filter(IceServer::isTurn) .filter(IceServer::isAuto) .forEach(server -> { server.setEnabled(server.hasAcceptableLatency(options.getAcceptableLatency())); }); + } catch (Exception e) { + log.error("Error in ICE server check", e); + } }ice-adapter/src/main/java/com/faforever/iceadapter/ui/IceWindow.java-39-52 (1)
39-52: NPE if FXML loading fails:new Scene(null)will throw.If
loader.load()throwsIOException,rootremainsnull. The subsequentnew Scene(root)on line 52 will throwNullPointerException. Either return early after catching the exception or propagate the error.🔎 Proposed fix
try { FXMLLoader loader = new FXMLLoader(getClass().getResource("/iceWindow.fxml")); root = loader.load(); controller = loader.getController(); controller.setAdapter(new UIAdapterImpl(IceAdapter.INSTANCE)); controller.initialize(); } catch (IOException e) { log.error("Could not load debugger window fxml", e); + return; // Prevent NPE from new Scene(null) }ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/other/PeerConnectivityCheckerModule.java-29-29 (1)
29-29: Resource leak:ScheduledExecutorServiceis never shut down.The
scheduledExecutorServicecreated at line 29 is never terminated. Thestop()method only cancels the scheduled future but doesn't shut down the executor. This leaks a thread per peer.🔎 Proposed fix
@Override public void stop() { if (!peer.isLocalOffer()) { return; } LockUtil.executeWithLock(peer.getLock(LOCK_CHECKER_MODULE), () -> { if (scheduledFuture != null) { scheduledFuture.cancel(true); scheduledFuture = null; } + scheduledExecutorService.shutdownNow(); }); }Also applies to: 74-85
ice-adapter/src/main/java/com/faforever/iceadapter/ui/controller/IceServerController.java-42-42 (1)
42-42: Resource leak:updateScheduleris never shut down.The
ScheduledExecutorServicecreated instartPeriodicUpdates()is never terminated. This will prevent the JVM from shutting down cleanly and leak threads. Add adispose()method similar toWindowController.🔎 Proposed fix
private UIAdapter adapter; private ScheduledExecutorService updateScheduler; + + public void dispose() { + if (updateScheduler != null && !updateScheduler.isShutdown()) { + updateScheduler.shutdown(); + } + }Also applies to: 105-110
ice-adapter/src/main/java/com/faforever/iceadapter/ui/controller/WindowController.java-211-221 (1)
211-221: Listener leak: value listeners accumulate on combo boxes.Each time
setSelectedPeeris called, new listeners are added toallowCombinationComboBoxandconnectionStrategyComboBoxwithout removing previous ones. This causes listener accumulation and potential memory leaks when users repeatedly select different peers.🔎 Proposed fix
Store the listener references as fields and remove them before adding new ones:
+ private javafx.beans.value.ChangeListener<AllowCombination> combinationListener; + private javafx.beans.value.ChangeListener<IceAgentStrategy> strategyListener; + private void setSelectedPeer(PeerView peer) { // ... existing code ... + // Remove previous listeners + if (combinationListener != null) { + allowCombinationComboBox.valueProperty().removeListener(combinationListener); + } + if (strategyListener != null) { + connectionStrategyComboBox.valueProperty().removeListener(strategyListener); + } + - allowCombinationComboBox.valueProperty().addListener((observable, oldValue, newValue) -> { + combinationListener = (observable, oldValue, newValue) -> { if (!Objects.equals(oldValue, newValue)) { adapter.setAllowCombination(peer, newValue); } - }); + }; + allowCombinationComboBox.valueProperty().addListener(combinationListener); - connectionStrategyComboBox.valueProperty().addListener((observable, oldValue, newValue) -> { + strategyListener = (observable, oldValue, newValue) -> { if (!Objects.equals(oldValue, newValue)) { adapter.setStrategy(peer, newValue); } - }); + }; + connectionStrategyComboBox.valueProperty().addListener(strategyListener);ice-adapter/src/main/java/com/faforever/iceadapter/services/impl/UIAdapterImpl.java-153-163 (1)
153-163: Potential listener memory leak:PeerViewregistered but never unregistered.When
toPeerInfois called, thePeerViewis registered as an event listener on thePeer(line 156). However, when peers are removed from the session (lines 113-117), the correspondingPeerViewis removed fromuiPeersbut never unregistered from thePeer's listener list.Consider unregistering the listener when removing peers:
uiPeers.keySet().stream() .filter(id -> !ids.contains(id)) .forEach(id -> { PeerView view = uiPeers.remove(id); // Unregister from peer if peer still exists getGameSession() .flatMap(s -> s.getPeer(id)) .ifPresent(p -> p.removeEventListener(view)); });ice-adapter/src/main/java/com/faforever/iceadapter/dto/PeerView.java-46-59 (1)
46-59: Thread safety concern: JavaFX properties modified outside FX Application Thread.The event callbacks (
onIceStateChange,onAgentChange,onLastPacketReceived) delegate toupdate(Peer), which modifies JavaFX properties. These callbacks are invoked asynchronously fromEventBusModulevia virtual threads (not the FX Application Thread). JavaFX properties should only be modified on the FX Application Thread to avoid potential race conditions or UI binding issues.Consider wrapping property updates in
Platform.runLater():public void update(Peer peer) { Platform.runLater(() -> { getConnected().set(peer.isConnected()); // ... rest of property updates }); }Also applies to: 61-89
ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/ice/PeerToPeerListenerModule.java-44-44 (1)
44-44: Lock acquisition wraps blocking receive loop.The
createListenermethod is called insideLockUtil.executeWithLock(), meaning the lock is held for the entire duration of the blocking receive loop. This could block other operations needing the same lock (e.g.,stop()).Consider restructuring to release the lock after setup:
Thread.ofVirtual().name(threadComponentListenerName()).start(() -> createListener(component));And move lock usage to only protect socket access/mutation if needed.
ice-adapter/src/main/java/com/faforever/iceadapter/services/impl/UIAdapterImpl.java-124-130 (1)
124-130: Creates newObservableListon each call.
getPeerInfoList()returns a newFXCollections.observableArrayList()each time it's called. This breaks JavaFX bindings since the list identity changes. If UI components bind to this list, they won't see updates.Consider maintaining a persistent
ObservableListfield that is updated in place.ice-adapter/src/main/java/com/faforever/iceadapter/ice/GameSession.java-38-38 (1)
38-38: Static mutable listiceServersis not thread-safe.The static
iceServerslist is modified bysetIceServers()(clear, addAll) without synchronization, while it may be read concurrently bygetIceServers()orIceServerChecker. This can causeConcurrentModificationExceptionor inconsistent reads.🔎 Proposed fix - use synchronized list or copy-on-write
- private static final List<IceServer> iceServers = createIceServers(); + private static final List<IceServer> iceServers = Collections.synchronizedList(new ArrayList<>()); + + static { + addDefaultIceServers(iceServers); + } public static void setIceServers(List<Map<String, Object>> iceServersData) { - iceServers.clear(); - addDefaultIceServers(iceServers); - ... - iceServers.addAll(pair.getFirst()); + synchronized (iceServers) { + iceServers.clear(); + addDefaultIceServers(iceServers); + ... + iceServers.addAll(pair.getFirst()); + }Alternatively, use
CopyOnWriteArrayListif reads vastly outnumber writes.Also applies to: 165-180
ice-adapter/src/main/java/com/faforever/iceadapter/rpc/RPCService.java-68-72 (1)
68-72: Potential NPE whengetPeerOrWait()returns null.
getPeerOrWait()can returnnull(line 118), but the callers at lines 70, 76, 86, 95-97, and 103 invoke methods on the result without null-checking, which will cause aNullPointerException.🔎 Proposed fix
public void onConnectionStateChanged(String newState) { if (!skipRPCMessages) { - getPeerOrWait().sendNotification("onConnectionStateChanged", List.of(newState)); + JJsonPeer peer = getPeerOrWait(); + if (peer != null) { + peer.sendNotification("onConnectionStateChanged", List.of(newState)); + } } }Apply similar null checks to the other methods.
ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/Peer.java-259-273 (1)
259-273: Race condition inclose()method.The check-then-act pattern on
closing(line 260-266) is not atomic. Two threads could both pass theif (closing)check before either setsclosing = true, causing duplicate close operations.🔎 Proposed fix using synchronized or AtomicBoolean
+ private final AtomicBoolean closingFlag = new AtomicBoolean(false); + public void close() { - if (closing) { + if (!closingFlag.compareAndSet(false, true)) { return; } log.info("Closing peer for player {}", getPeerIdentifier()); - closing = true; - event(bus -> bus.onClose(this, closing)); + event(bus -> bus.onClose(this, true)); for (ModuleBase module : modules.values()) { module.stop(); } log.info("Peer closed: {}", getPeerIdentifier()); }Note: Also update the
isClosing()method to useclosingFlag.get().Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (24)
ice-adapter/src/main/java/com/faforever/iceadapter/util/DatagramSocketUtils.java-25-36 (1)
25-36: STUN packet type validation contains incorrect and obsolete type codes.The method checks several STUN message types, but some are invalid per RFC 5389:
0x0000: Invalid—method 0 is not defined in any STUN RFC0x0002: Invalid—should be0x0011for Binding Indication (RFC 5389)0x0115/0x0116: Obsolete—these method values (0x045) are from RFC 3489, not RFC 5389RFC 5389 defines valid Binding method types with correct class bits:
0x0001: Binding Request0x0101: Binding Success Response0x0111: Binding Error Response0x0011: Binding IndicationUpdate the type checks to match RFC 5389 specifications.
ice-adapter/src/main/java/com/faforever/iceadapter/util/ExecutorHolder.java-10-12 (1)
10-12: Clarify or verify the Ice4J virtual thread constraint in javadoc.The javadoc states Ice4J is "not allowed to use virtual threads in Java 21," but this appears to be an unverified assumption rather than an established constraint. Ice4J is a standard JVM library that runs on Java 21; no official project documentation explicitly forbids virtual threads. The actual issue is that virtual thread compatibility is untested and depends on Ice4J's internal use of ThreadLocals, synchronized blocks, or native operations.
Either cite a specific Ice4J issue/version requirement for this constraint, or revise the javadoc to reflect that virtual thread compatibility is untested and requires validation.
ice-adapter/src/main/java/com/faforever/iceadapter/util/ExecutorHolder.java-16-17 (1)
16-17: Add explicit shutdown for executors if the application needs graceful cleanup.While the executors are never explicitly shut down, this has minimal practical impact since the application always calls
System.exit()which terminates the JVM immediately. However, for cleaner shutdown semantics and to avoid abruptly terminating threads, consider adding ashutdown()method toExecutorHolderthat properly closes both executors before application exit.ice-adapter/src/main/java/com/faforever/iceadapter/IceOptions.java-52-55 (1)
52-55: Description appears to be copy-pasted from--ping-count.The description for
--acceptable-latencysays "number of times to ping each turn server to determine latency", which is the description for--ping-count. This should describe what the acceptable latency threshold is used for.Suggested fix
@Option(names = "--acceptable-latency", defaultValue = "250.0", - description = "number of times to ping each turn server to determine latency") + description = "maximum acceptable latency threshold in milliseconds") private double acceptableLatency;client/src/main/java/client/ice/ICEAdapter.java-223-223 (1)
223-223: Hardcoded game-id is inconsistent with other dynamic parameters.The
--game-idis set to a hardcoded value100, while--idand--loginare dynamically sourced fromTestClient.playerIDandTestClient.username. Add a comment explaining if this is intentional test scaffolding, or make it configurable to match the pattern of other parameters.ice-adapter/src/main/java/com/faforever/iceadapter/ice/IceServerChecker.java-23-31 (1)
23-31: Race condition instart(): double scheduling possible.If
start()is called concurrently from multiple threads, both may seescheduledFuture == nulland schedule duplicate tasks. Consider synchronizing access or usingAtomicReference<ScheduledFuture<?>>with CAS.🔎 Proposed fix with synchronization
+ private final Object lock = new Object(); + public void start() { - if (scheduledFuture != null) { - return; + synchronized (lock) { + if (scheduledFuture != null) { + return; + } + scheduledFuture = scheduler.scheduleAtFixedRate(this::checkerThread, + 0, + INTERVAL, + TimeUnit.MINUTES); } - scheduledFuture = scheduler.scheduleAtFixedRate(this::checkerThread, - 0, - INTERVAL, - TimeUnit.MINUTES); }Committable suggestion skipped: line range outside the PR's diff.
ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/other/AutoSettingAllowCandidates.java-35-43 (1)
35-43: Race condition in combination cycling logic.The
getAndIncrement()followed by a conditional reset can cause multiple threads to cycle incorrectly if called concurrently. UseupdateAndGetwith modulo arithmetic for atomic cycling.🔎 Suggested fix
private void changeCombination() { - int id = index.getAndIncrement(); - if (id >= combinations.size()) { - index.set(0); - id = 0; - } + int id = index.updateAndGet(i -> (i + 1) % combinations.size()); AllowCombination combination = combinations.get(id); peer.setCombination(combination); }Note: This changes the starting index behavior slightly (starts at 1 instead of 0), so adjust if the first combination must be index 0:
int id = index.getAndUpdate(i -> (i + 1) % combinations.size());ice-adapter/src/main/java/com/faforever/iceadapter/ui/IceServerWindow.java-74-81 (1)
74-81: Race condition inlaunch()singleton initialization.Multiple concurrent calls to
launch()could pass theINSTANCE == nullcheck before any of them completesstart(), potentially creating multiple windows. Consider synchronizing the check-and-create logic.🔎 Suggested fix
public static void launch() { log.info("Launching IceServerTableApp window."); - if (INSTANCE == null) { - runOnUIThread(() -> new IceServerWindow().start(new Stage())); - } else { - INSTANCE.showWindow(); - } + runOnUIThread(() -> { + if (INSTANCE == null) { + new IceServerWindow().start(new Stage()); + } else { + INSTANCE.showWindow(); + } + }); }ice-adapter/build.gradle-34-34 (1)
34-34: Test ice4j 3.2 compatibility thoroughly before merging.The upgrade from the custom ice4j fork (
3.0-66-g1c60acc) to the official Jitsi mainline (3.2-12-gc2cbf61) is a significant migration. Ensure the existing test suite (particularlyIceTest.java) passes and validate ICE connectivity functionality in realistic scenarios, as the codebase has multiple dependencies on ice4j APIs acrossConnectServiceCommon,ConnectServiceControlled/NotControlled, and related classes.ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/PeerModule.java-62-66 (1)
62-66: Redundant comparator: compares the same field twice.The
thenComparing(PeerModule::getPriority)adds no value since it's comparing the same field that was already used incomparingInt. This appears to be a copy-paste error.🔎 Proposed fix
@Override public int compare(PeerModule o1, PeerModule o2) { - return Comparator.comparingInt(PeerModule::getPriority) - .thenComparing(PeerModule::getPriority).compare(o1, o2); + return Integer.compare(o1.getPriority(), o2.getPriority()); }ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/other/PeerConnectivityCheckerModule.java-106-107 (1)
106-107: Bug: Log message uses wrong variable.The log message prints
lastPacketReceived(a timestamp) instead ofsinceLastReal(the elapsed time). This will display confusing values like "No traffic for 1703372400000 ms".🔎 Proposed fix
if (sinceLastReal > TIMEOUT_BEFORE_LOST_CONNECT) { - log.warn("{} No traffic (echo or game) for {} ms (> {} ms timeout). Closing connection.", - peer.getPeerIdentifier(), lastPacketReceived, TIMEOUT_BEFORE_LOST_CONNECT); + log.warn("{} No traffic (echo or game) for {} ms (> {} ms timeout). Closing connection.", + peer.getPeerIdentifier(), sinceLastReal, TIMEOUT_BEFORE_LOST_CONNECT); peer.lostConnect(); return; }ice-adapter/src/main/java/com/faforever/iceadapter/ui/controller/IceServerController.java-64-68 (1)
64-68: PotentialIndexOutOfBoundsExceptionwhen accessing table items.The
getIndex()call inside the CheckBox action handler can throw an exception if the table items are modified concurrently (e.g., during a periodic update). Consider adding a bounds check or using the cell's current item instead.🔎 Proposed fix
{ checkBox.setOnAction(event -> { - IceServerView item = getTableView().getItems().get(getIndex()); - adapter.setEnabledIceServer(item, checkBox.isSelected()); + int index = getIndex(); + if (index >= 0 && index < getTableView().getItems().size()) { + IceServerView item = getTableView().getItems().get(index); + adapter.setEnabledIceServer(item, checkBox.isSelected()); + } }); }ice-adapter/src/main/java/com/faforever/iceadapter/ui/controller/WindowController.java-248-264 (1)
248-264: Inconsistent threading: label updates outsidePlatform.runLater.The label updates (lines 253-263) are performed directly while the peer table update is wrapped in
Platform.runLater. SinceupdateAllInfois called from a background scheduler thread, all UI updates should be withinPlatform.runLaterto avoid potential threading issues.🔎 Proposed fix
private void updateAllInfo() { if (adapter == null) { return; } - versionLabel.setText("Version: %s".formatted(adapter.getVersion())); - userLabel.setText("User: %s(%s)".formatted(adapter.getUsername(), adapter.getUserId())); - rpcPortLabel.setText("RPC_PORT: %s".formatted(adapter.getRpcPort())); - gpgnetPortLabel.setText("GPGNET_PORT: %s".formatted(adapter.getGpgNetPort())); - lobbyPortLabel.setText("LOBBY_PORT: %s".formatted(adapter.getLobbyPort())); - - rpcServerStatus.setText("RPCServer: %s".formatted(adapter.getRpcServerStatus())); - rpcClientStatus.setText("RPCClient: %s".formatted(adapter.getRpcClientStatus())); - gpgnetServerStatus.setText("GPGNetServer: %s".formatted(adapter.getGpgNetServerStatus())); - gpgnetClientStatus.setText("GPGNetClient: %s".formatted(adapter.getGpgNetClientStatus())); - gameState.setText("GameState: %s".formatted(adapter.getGameState())); - Platform.runLater(() -> { + versionLabel.setText("Version: %s".formatted(adapter.getVersion())); + userLabel.setText("User: %s(%s)".formatted(adapter.getUsername(), adapter.getUserId())); + rpcPortLabel.setText("RPC_PORT: %s".formatted(adapter.getRpcPort())); + gpgnetPortLabel.setText("GPGNET_PORT: %s".formatted(adapter.getGpgNetPort())); + lobbyPortLabel.setText("LOBBY_PORT: %s".formatted(adapter.getLobbyPort())); + + rpcServerStatus.setText("RPCServer: %s".formatted(adapter.getRpcServerStatus())); + rpcClientStatus.setText("RPCClient: %s".formatted(adapter.getRpcClientStatus())); + gpgnetServerStatus.setText("GPGNetServer: %s".formatted(adapter.getGpgNetServerStatus())); + gpgnetClientStatus.setText("GPGNetClient: %s".formatted(adapter.getGpgNetClientStatus())); + gameState.setText("GameState: %s".formatted(adapter.getGameState())); + PeerView currentlySelected = peerTable.getSelectionModel().getSelectedItem(); // ... rest of code }); }Committable suggestion skipped: line range outside the PR's diff.
ice-adapter/src/main/java/com/faforever/iceadapter/ui/InfoWindow.java-76-83 (1)
76-83: Race condition inlaunch()singleton initialization.The check
if (INSTANCE == null)and subsequent creation is not thread-safe. Iflaunch()is called concurrently from multiple threads, multipleInfoWindowinstances could be created.Consider synchronizing or using a holder pattern:
🔎 Proposed fix
public static void launch() { log.info("Launching info window."); - if (INSTANCE == null) { - runOnUIThread(() -> new InfoWindow().start(new Stage())); - } else { - INSTANCE.showWindow(); - } + runOnUIThread(() -> { + if (INSTANCE == null) { + new InfoWindow().start(new Stage()); + } else { + INSTANCE.showWindow(); + } + }); }This ensures the null check and creation happen on the same thread.
ice-adapter/src/main/java/com/faforever/iceadapter/services/impl/ConnectServiceControlledImpl.java-72-78 (1)
72-78: Missing closing check before scheduling re-init.
tryReInitStateschedules a delayed state transition toNEWbut doesn't check if the peer is closing. This could cause issues if the peer closes before the delay completes.🔎 Proposed fix
private void tryReInitState(Peer peer, IceState oldState) { + if (peer.isClosing()) { + return; + } if (oldState == CONNECTED) { - iceAsync.runAsyncDelay(peer, () -> peer.setIceState(NEW), 1000); + iceAsync.runAsyncDelay(peer, () -> { + if (!peer.isClosing()) peer.setIceState(NEW); + }, 1000); } else { - iceAsync.runAsyncDelay(peer, () -> peer.setIceState(NEW), 5000); + iceAsync.runAsyncDelay(peer, () -> { + if (!peer.isClosing()) peer.setIceState(NEW); + }, 5000); } }ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/other/FASocketModule.java-41-55 (1)
41-55: Logging mismatch and redundant@SneakyThrows.
Line 49 logs
peer.getLocalPort()but the actual port used is the resolvedportvariable. IflocalPortwasnull, the log will be misleading.
@SneakyThrows(SocketException.class)is unnecessary since the exception is caught and explicitly rethrown within the method.🔎 Proposed fix
- @SneakyThrows(SocketException.class) private DatagramSocket initForwarding(int port, Integer localPort) { try { if (localPort != null) { port = localPort; } DatagramSocket socket = new DatagramSocket(port); DatagramSocketUtils.resizeBuffer(socket); - log.debug("Now forwarding data to peer {} on port {}", peer.getPeerIdentifier(), peer.getLocalPort()); + log.debug("Now forwarding data to peer {} on port {}", peer.getPeerIdentifier(), port); return socket; } catch (SocketException e) { log.error("Could not create socket for peer: {}", peer.getPeerIdentifier(), e); throw e; } }ice-adapter/src/main/java/com/faforever/iceadapter/ice/peer/modules/EventBusModule.java-94-102 (1)
94-102:safeAsyncCallis not actually "safe" - missing exception handling.The method name suggests exception safety, but exceptions thrown by listeners will propagate uncaught in virtual threads. Consider wrapping the call in a try-catch:
🔎 Proposed fix
private void safeAsyncCall(PeerEventListener listener, String methodName, Runnable call) { - asyncVirtual(methodName, peer, call); + asyncVirtual(methodName, peer, () -> { + try { + call.run(); + } catch (Exception e) { + log.error("Error in listener {} during {}", listener.getClass().getSimpleName(), methodName, e); + } + }); }ice-adapter/src/main/java/com/faforever/iceadapter/ui/InfoWindow.java-30-42 (1)
30-42:INSTANCEset before FXML loading could leave partial state on failure.If
loader.load()throws an exception (line 38),INSTANCEis already set tothis(line 31), leaving a partially initialized singleton. Subsequentlaunch()calls would try to show a broken window.Consider setting
INSTANCEafter successful initialization:🔎 Proposed fix
public void start(Stage stage) { - INSTANCE = this; this.stage = stage; LogoUtils.getLogoFx().ifPresent(logo -> { stage.getIcons().add(logo); }); try { FXMLLoader loader = new FXMLLoader(getClass().getResource("/infoWindow.fxml")); root = loader.load(); controller = loader.getController(); } catch (IOException e) { log.error("Could not load debugger window fxml", e); + return; // Don't set INSTANCE on failure } setUserAgentStylesheet(STYLESHEET_MODENA); // ... rest of setup + INSTANCE = this;Committable suggestion skipped: line range outside the PR's diff.
ice-adapter/src/main/java/com/faforever/iceadapter/services/impl/ConnectServiceControlledImpl.java-102-111 (1)
102-111: Use peer'sAllowCombinationsettings instead of hardcodedtruevalues.Both
ConnectServiceControlledImplandConnectServiceNotControlledImplcallunpackCandidateswith all three boolean parameters hardcoded totrue. This is inconsistent withConnectServiceCommon, which filters outgoing candidates usingpeer.getCombination(). Apply the same combination-based filtering when unpacking incoming candidates for consistency.ice-adapter/src/main/java/com/faforever/iceadapter/IceAdapter.java-178-204 (1)
178-204:close(int status)accesses instance fields without synchronization.The method reads
INSTANCEinto a local variable, but subsequent access toinstance.gpgNetServerandinstance.rpcServiceis not synchronized. If another thread modifies these fields concurrently, inconsistent state could result. Consider synchronizing or making these fields final.ice-adapter/src/main/java/com/faforever/iceadapter/services/impl/ConnectServiceCommon.java-268-281 (1)
268-281: PotentialConcurrentModificationExceptionwhen iterating and removing streams.The loop iterates
agent.getStreams()while callingremoveStream()andremoveComponent()inside. Depending on the implementation ofgetStreams(), this could throwConcurrentModificationException.🔎 Proposed fix - copy collection before iterating
protected void closeAgent(Agent agent) { log.info("Close agent"); try { - for (IceMediaStream stream : agent.getStreams()) { - for (Component streamComponent : stream.getComponents()) { + for (IceMediaStream stream : new ArrayList<>(agent.getStreams())) { + for (Component streamComponent : new ArrayList<>(stream.getComponents())) { stream.removeComponent(streamComponent); } agent.removeStream(stream); } agent.free(); } catch (Exception e) { log.warn("Error freeing existing agent", e); } }ice-adapter/src/main/java/com/faforever/iceadapter/ice/IceServer.java-85-89 (1)
85-89: Unchecked cast and incorrect fallback key.Line 86 performs an unchecked cast to
List<String>. Line 88 falls back toiceServerData.get("url")(singular) but the enclosingifchecks for"urls"(plural), which means the fallback will never retrieve a value when"urls"key exists.🔎 Proposed fix
if (iceServerData.containsKey("urls")) { List<String> urls; Object urlsData = iceServerData.get("urls"); if (urlsData instanceof List) { - urls = (List<String>) urlsData; + @SuppressWarnings("unchecked") + List<String> castUrls = (List<String>) urlsData; + urls = castUrls; } else { - urls = Collections.singletonList((String) iceServerData.get("url")); + urls = Collections.singletonList((String) urlsData); }ice-adapter/src/main/java/com/faforever/iceadapter/services/impl/ConnectServiceCommon.java-206-221 (1)
206-221: Double timeout may cause extended waits.
PeerConnectionSuccessMonitoralready has aTIMEOUT_ON_CHECKINGtimeout (line 209), and thenfuture.get()at line 214 adds anotherTIMEOUT_ON_CHECKINGwait. In the worst case, this could wait up to 30 seconds instead of the intended 15.🔎 Proposed fix - use longer timeout for get() as safety net
try { - return future.get(TIMEOUT_ON_CHECKING, TimeUnit.MILLISECONDS); + // Use a slightly longer timeout as a safety net; monitor handles the actual timeout + return future.get(TIMEOUT_ON_CHECKING + 1000, TimeUnit.MILLISECONDS); } catch (Exception e) {ice-adapter/src/main/java/com/faforever/iceadapter/ice/IceServer.java-115-120 (1)
115-120: Default case logs warning but still creates IceServer with TURN type.When an unknown scheme is encountered, the code logs a warning but proceeds to create an
IceServerwithtype = TypeServer.TURN(initialized at line 115). Consider skipping the entry entirely or explicitly setting a default type after logging.🔎 Proposed fix to skip unknown schemes
TypeServer type = TypeServer.TURN; switch (uri.getScheme()) { case STUN -> type = TypeServer.STUN; case TURNS, TURN -> type = TypeServer.TURN; - default -> log.warn("Invalid ICE server protocol: {}", uri); + default -> { + log.warn("Invalid ICE server protocol: {}", uri); + return; // Skip this URI + } }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| // perform the potentially blocking and cross-module shutdown outside of the lock to avoid deadlocks | ||
| if (currentClient.get() != null) { | ||
| iceAdapter.onFAShutdown(); | ||
| debug().gpgnetConnectedDisconnected(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical logic bug: FA shutdown never triggered.
After currentClient.getAndSet(null) at line 246, the check at line 255 if (currentClient.get() != null) will always be false because you just set it to null. The FA shutdown and debug notification will never execute.
🔎 Proposed fix
private void onGpgnetConnectionLost() {
log.info("GPGNet connection lost");
+ boolean hadClient = false;
// remove and close the client under lock
LockUtil.executeWithLock(lockSocket, () -> {
GPGNetClient prevClient = currentClient.getAndSet(null);
if (prevClient != null) {
+ hadClient = true;
prevClient.close();
- // create a fresh lobby future if necessary is handled per-client
rpcService.onConnectionStateChanged("Disconnected");
}
});
// perform the potentially blocking and cross-module shutdown outside of the lock to avoid deadlocks
- if (currentClient.get() != null) {
+ if (hadClient) {
iceAdapter.onFAShutdown();
debug().gpgnetConnectedDisconnected();
}
}Note: You'll need to use an AtomicBoolean or similar since the variable is accessed from within a lambda.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ice-adapter/src/main/java/com/faforever/iceadapter/gpgnet/GPGNetServer.java
around lines 253 to 259, the code checks currentClient.get() after calling
currentClient.getAndSet(null), so the condition is always false and FA shutdown
never runs; fix by capturing the previous client state before clearing (e.g.,
Client previous = currentClient.getAndSet(null)) and then if (previous != null)
call iceAdapter.onFAShutdown() and debug().gpgnetConnectedDisconnected();
alternatively, if you need a boolean flag for lambda/thread use, replace or
accompany with an AtomicBoolean to record whether a non-null client was present
and check that flag after the atomic set.
| public CompletableFuture<Boolean> start() { | ||
| future = new CompletableFuture<>(); | ||
| if (peer == null) { | ||
| log.error("Peer is null. Aborting."); | ||
| future.complete(false); | ||
| return future; | ||
| } | ||
| Agent agent = peer.getAgent(); | ||
| if (agent == null) { | ||
| log.error("Agent is null. Aborting."); | ||
| future.complete(false); | ||
| return future; | ||
| } | ||
| IceMediaStream mediaStream = peer.getMediaStream(); | ||
| if (mediaStream == null) { | ||
| log.error("Media stream is null. Aborting."); | ||
| future.complete(false); | ||
| return future; | ||
| } | ||
|
|
||
| name = peer.getPeerIdentifier(); | ||
|
|
||
| agent.addStateChangeListener(this); | ||
| mediaStream.addPairChangeListener(this); | ||
|
|
||
| scheduledFuture = scheduler.schedule(this::agentTimeoutFailed, timeoutMs, TimeUnit.MILLISECONDS); | ||
|
|
||
| return future; | ||
| } |
There was a problem hiding this comment.
Memory leak: listeners never unregistered; start() not idempotent.
Two critical issues:
-
Listener leak (lines 48-49): The monitor registers itself as a listener on both
agentandmediaStream, but these listeners are never unregistered—not on completion, failure, timeout, or even inshutdown(). If the agent/mediaStream outlive this monitor, these listeners will leak memory. -
Not idempotent: If
start()is called multiple times, a newfutureis created each time (line 27), losing the reference to previous futures and their callbacks. Additionally, listeners are registered multiple times without deregistration.
🔎 Proposed fix
+ private Agent agent;
+ private IceMediaStream mediaStream;
+
@Override
public CompletableFuture<Boolean> start() {
+ if (future != null && !future.isDone()) {
+ log.warn("start() called while already running. Returning existing future.");
+ return future;
+ }
+
future = new CompletableFuture<>();
if (peer == null) {
log.error("Peer is null. Aborting.");
future.complete(false);
return future;
}
- Agent agent = peer.getAgent();
+ agent = peer.getAgent();
if (agent == null) {
log.error("Agent is null. Aborting.");
future.complete(false);
return future;
}
- IceMediaStream mediaStream = peer.getMediaStream();
+ mediaStream = peer.getMediaStream();
if (mediaStream == null) {
log.error("Media stream is null. Aborting.");
future.complete(false);
return future;
}
name = peer.getPeerIdentifier();
agent.addStateChangeListener(this);
mediaStream.addPairChangeListener(this);
scheduledFuture = scheduler.schedule(this::agentTimeoutFailed, timeoutMs, TimeUnit.MILLISECONDS);
+ future.whenComplete((result, ex) -> cleanup());
return future;
}
+
+ private void cleanup() {
+ if (scheduledFuture != null && !scheduledFuture.isDone()) {
+ scheduledFuture.cancel(false);
+ }
+ if (agent != null) {
+ agent.removeStateChangeListener(this);
+ }
+ if (mediaStream != null) {
+ mediaStream.removePairChangeListener(this);
+ }
+ }Also update shutdown() to call cleanup().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ice-adapter/src/main/java/com/faforever/iceadapter/ice/PeerConnectionSuccessMonitor.java
around lines 26 to 54, start() currently recreates the CompletableFuture and
re-registers listeners every call and never removes listeners, causing leaks and
non-idempotency; make start() idempotent by returning the existing future if
already started, avoid registering listeners a second time, and only create the
future once. Implement a private cleanup() that unregisters this from agent and
mediaStream, cancels scheduledFuture, and nulls out/cleans state; call cleanup()
on success, failure, timeout and from shutdown(). Ensure scheduledFuture is
cancelled when finished and the future is completed exactly once.
|
|
||
| public class CandidateUtil { | ||
|
|
||
| public static int candidateIDFactory = 0; |
There was a problem hiding this comment.
Race condition on shared mutable static field.
candidateIDFactory is a static mutable field incremented without synchronization (line 34). If packCandidates is called concurrently from multiple threads, this will cause race conditions leading to duplicate candidate IDs or lost increments.
🔎 Proposed fix using AtomicInteger
At line 15, replace the field declaration:
- public static int candidateIDFactory = 0;
+ private static final AtomicInteger candidateIDFactory = new AtomicInteger(0);At line 34, update the usage:
- String.valueOf(candidateIDFactory++),
+ String.valueOf(candidateIDFactory.getAndIncrement()),Add the import at the top of the file:
import java.util.concurrent.atomic.AtomicInteger;Also applies to: 34-34
🤖 Prompt for AI Agents
In ice-adapter/src/main/java/com/faforever/iceadapter/util/CandidateUtil.java
around lines 15 and 34, the static mutable int candidateIDFactory is incremented
without synchronization causing race conditions; replace the declaration with a
static AtomicInteger (add import java.util.concurrent.atomic.AtomicInteger), and
at the usage site replace the plain increment with atomic getAndIncrement() (or
incrementAndGet() as appropriate) so candidate IDs are produced atomically and
safely under concurrent calls.
| public static String infoCandidate(CandidatePair pair) { | ||
| if (pair == null) { | ||
| return null; | ||
| } | ||
| return """ | ||
| Local Candidate: | ||
| Type: %s | ||
| Transport: %s | ||
| Address: %s:%d | ||
| Priority: %d | ||
| Foundation: %s | ||
| Remote Candidate: | ||
| Type: %s | ||
| Transport: %s | ||
| Address: %s:%d | ||
| Priority: %d | ||
| Foundation: %s | ||
| Priority: %d | ||
| Nominated: %s | ||
| State: %s | ||
| """.formatted( | ||
| pair.getLocalCandidate().getType(), | ||
| pair.getLocalCandidate().getTransport(), | ||
| pair.getLocalCandidate().getTransportAddress().getHostAddress(), | ||
| pair.getLocalCandidate().getTransportAddress().getPort(), | ||
| pair.getLocalCandidate().getPriority(), | ||
| pair.getLocalCandidate().getFoundation(), | ||
| pair.getRemoteCandidate().getType(), | ||
| pair.getLocalCandidate().getTransport(), | ||
| pair.getRemoteCandidate().getTransportAddress().getHostAddress(), | ||
| pair.getRemoteCandidate().getTransportAddress().getPort(), | ||
| pair.getRemoteCandidate().getPriority(), | ||
| pair.getRemoteCandidate().getFoundation(), | ||
| pair.getPriority(), | ||
| pair.isNominated(), | ||
| pair.getState() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix copy-paste error: displaying wrong transport for remote candidate.
Line 144 incorrectly uses pair.getLocalCandidate().getTransport() in the Remote Candidate section. It should use pair.getRemoteCandidate().getTransport() instead.
🔎 Proposed fix
pair.getRemoteCandidate().getType(),
- pair.getLocalCandidate().getTransport(),
+ pair.getRemoteCandidate().getTransport(),
pair.getRemoteCandidate().getTransportAddress().getHostAddress(),🤖 Prompt for AI Agents
In ice-adapter/src/main/java/com/faforever/iceadapter/util/CandidateUtil.java
around lines 114 to 153, there is a copy-paste bug in the formatted argument
list: the Remote Candidate's Transport value is passed as
pair.getLocalCandidate().getTransport() instead of
pair.getRemoteCandidate().getTransport(); replace that specific argument with
pair.getRemoteCandidate().getTransport() so the Remote Candidate section shows
the correct transport while leaving all other format placeholders and arguments
unchanged.
| if (System.getProperty("os.name").startsWith("Windows")) { | ||
| process = new ProcessBuilder("ping", "-n", count.toString(), address).start(); | ||
| // Force English output using code page 437 | ||
| String command = String.format("chcp 437 > NUL && ping -n %d %s", count, address); | ||
| process = new ProcessBuilder("cmd", "/c", command).start(); |
There was a problem hiding this comment.
Potential command injection vulnerability.
The address parameter is directly interpolated into the shell command string without validation or sanitization. A malicious address like example.com & malicious_command could execute arbitrary commands.
🔎 Proposed fix to prevent command injection
if (System.getProperty("os.name").startsWith("Windows")) {
// Force English output using code page 437
- String command = String.format("chcp 437 > NUL && ping -n %d %s", count, address);
- process = new ProcessBuilder("cmd", "/c", command).start();
+ process = new ProcessBuilder("cmd", "/c", "chcp", "437", ">", "NUL", "&&", "ping", "-n", count.toString(), address).start();
output_pattern = WINDOWS_OUTPUT_PATTERN;Even better, separate the commands:
if (System.getProperty("os.name").startsWith("Windows")) {
// Force English output using code page 437
- String command = String.format("chcp 437 > NUL && ping -n %d %s", count, address);
- process = new ProcessBuilder("cmd", "/c", command).start();
+ // Set code page in environment instead
+ ProcessBuilder pb = new ProcessBuilder("ping", "-n", count.toString(), address);
+ pb.environment().put("CHCP", "437");
+ process = pb.start();
output_pattern = WINDOWS_OUTPUT_PATTERN;Note: The second approach may require testing to confirm code page handling works as expected.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ice-adapter/src/main/java/com/faforever/iceadapter/util/PingWrapper.java
around lines 30-33, the address variable is being interpolated into a shell
command which allows command injection; fix by (1) never passing a single
concatenated command string to the shell with untrusted input — run chcp and
ping as separate ProcessBuilder invocations or invoke ping directly without cmd
/c using ProcessBuilder with each argument as a separate element (no shell
parsing), and (2) validate/sanitize the address beforehand (restrict to a safe
regex for IPv4/IPv6 or hostname characters) and reject or throw on invalid
values so only validated addresses reach the ProcessBuilder.
| implementation("ch.qos.logback:logback-classic:1.5.6") | ||
| implementation("org.slf4j:slf4j-api:2.0.13") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for latest versions of logging dependencies
echo "=== Checking logback-classic latest version ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:ch.qos.logback+AND+a:logback-classic&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion' 2>/dev/null || echo "Failed to fetch"
echo ""
echo "=== Checking slf4j-api latest version ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:org.slf4j+AND+a:slf4j-api&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion' 2>/dev/null || echo "Failed to fetch"
echo ""
echo "=== Checking recent versions of logback-classic ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:ch.qos.logback+AND+a:logback-classic&rows=5&wt=json" | jq -r '.response.docs[] | .v' 2>/dev/null | head -10
echo ""
echo "=== Checking recent versions of slf4j-api ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:org.slf4j+AND+a:slf4j-api&rows=5&wt=json" | jq -r '.response.docs[] | .v' 2>/dev/null | head -10Repository: FAForever/java-ice-adapter
Length of output: 196
🌐 Web query:
logback-classic latest version 2025 Maven Central
💡 Result:
The latest logback-classic on Maven Central is ch.qos.logback:logback-classic:1.5.22 (published Dec 11, 2025). [1][2]
Sources:
- Maven Central directory for logback-classic (shows 1.5.22). [1]
- Logback project download/setup pages. [2]
🌐 Web query:
slf4j-api latest version 2025 Maven Central
💡 Result:
Latest stable slf4j-api on Maven Central: 2.0.17 (released Feb 25, 2025). [1][2]
Sources:
[1] SLF4J download page (stable: 2.0.17).
[2] Maven Central repository metadata for org.slf4j:slf4j-api.
🌐 Web query:
logback-classic 1.5.6 security vulnerabilities CVE
💡 Result:
Short answer: Yes — logback 1.5.6 is vulnerable to CVE-2025-11226 (arbitrary code execution in logback-core) and is in the affected range for that issue. Older serialization/DoS CVEs (CVE-2023-6378 / CVE-2023-6481) affected earlier 1.2/1.3/1.4 series but are separate. [1][2][3][4]
Details/sources:
- CVE-2025-11226 (ACE in conditional configuration processing) — affects logback-core up to and including 1.5.18 (so 1.5.6 is vulnerable). [NVD]. [1][2]
- CVE-2023-6378 and CVE-2023-6481 — serialization/DoS issues in receiver components affecting 1.2/1.3/1.4 series (separately disclosed in 2023). [3][4]
- Maven listing for logback-classic 1.5.6 (shows known vulnerability references). [5]
References:
[1] NVD / CVE-2025-11226. (nvd.nist.gov)
[2] GitHub Advisory / CVE-2025-11226. (github.com)
[3] GitHub Advisory / CVE-2023-6481 (serialization/DoS). (github.com)
[4] OSV / logback security bulletins (CVE-2023-6378, CVE-2023-6481). (osv.dev)
[5] Maven Repository — logback-classic 1.5.6 metadata. (mvnrepository.com)
If you want, I can (a) list exact affected version ranges per CVE, (b) show mitigation/workaround and fixed versions, or (c) produce a patch/upgrade plan.
Citations:
- 1: https://nvd.nist.gov/vuln/detail/CVE-2025-11226?utm_source=openai
- 2: https://github.com/advisories/GHSA-25qh-j22f-pwp8?utm_source=openai
- 3: https://github.com/advisories/GHSA-gm62-rw4g-vrc4?utm_source=openai
- 4: https://osv.dev/vulnerability/OESA-2023-1946?utm_source=openai
- 5: https://mvnrepository.com/artifact/ch.qos.logback/logback-classic/1.5.6?utm_source=openai
🌐 Web query:
slf4j-api 2.0.13 security vulnerabilities CVE
💡 Result:
Short answer: I found no published CVE that specifically names slf4j-api 2.0.13. [1][2]
Notes and relevant references:
- SLF4J release notes for 2.0.13 (no CVE listed for slf4j-api 2.0.13). [1]
- Historical CVE affecting SLF4J components: CVE‑2018‑8088 (slf4j-ext) — fixed in later 1.7.x and in the 2.0.x series. This CVE is not specific to slf4j-api 2.0.13. [2]
- Many CVEs in the Java logging ecosystem (e.g., Log4j-related CVEs, Jackson issues) can affect applications that use SLF4J depending on the bound implementation; check your logging binding (log4j, logback, etc.) and transitive dependencies. See SLF4J guidance on Log4Shell and ecosystem advisories. [3][4]
If you want, I can:
- run a targeted NVD/CVE feed lookup for a specific date range or CVE IDs, or
- scan your project's dependency tree to show whether any vulnerable logging implementations or transitive libraries are present.
Sources:
[1] SLF4J news / release 2.0.13.
[2] GitLab advisory / CVE‑2018‑8088 (slf4j‑ext).
[3] SLF4J guidance on Log4Shell and related risks.
[4] Snyk package advisories for SLF4J/log4j-slf4j related packages.
🌐 Web query:
logback-classic 1.5.6 slf4j-api 2.0.13 compatibility
💡 Result:
Short answer: Yes — logback-classic 1.5.6 (part of Logback 1.5.x) is intended to work with SLF4J 2.0.x, so it is compatible with slf4j-api 2.0.13. Use matching SLF4J major/minor (2.0) artifacts and avoid mixing different SLF4J major/minor versions. [1][2][3]
Sources:
- Logback dependencies / version matrix (Logback 1.5.x → SLF4J 2.0). [1]
- SLF4J FAQ / compatibility notes (providers must match SLF4J major.minor; 2.0.x details). [2]
- Maven Central listing for slf4j-api 2.0.13. [3]
Upgrade logging dependencies immediately due to CVE-2025-11226.
The logback-classic version 1.5.6 is vulnerable to CVE-2025-11226 (arbitrary code execution in conditional configuration processing). Upgrade to logback-classic 1.5.19 or later (current: 1.5.22). Also update slf4j-api to 2.0.17 (from 2.0.13). Both dependencies are compatible with each other.
🤖 Prompt for AI Agents
In server/build.gradle around lines 18 to 20, the project depends on vulnerable
logback-classic 1.5.6 and an older slf4j-api (2.0.13); update logback-classic to
a non-vulnerable release (>=1.5.19) — e.g. 1.5.22 — and bump slf4j-api to 2.0.17
so the two are compatible, then run a build and/or `./gradlew dependencies` to
verify the resolved versions and ensure no dependency conflicts.
1 participant
Fixs:
Fix pingwrapper for windows with a language other than "English".
Fixing the TCP parsing of FAF servers.
Features:
A complete rewrite of the Ice4J connection code for Peer2Peer.
Legacy code has been removed from the IceAdapter without loss of functionality.
Ice4J has been updated to the latest version.
Deleted/changed the order of HOST+STUN+RELAY -> STUN+RELAY -> RELAY -> always RELAY.
Now HOST/STUN/RELAY alternate in a cycle, which has a positive effect on connections for users who do not have RELAY working.
The Peer implementation is divided into blocks/modules with the ability to disable.
The Peer connection does not depend on modules, but modules can make their own changes to the connection if necessary.
The connection is implemented in services.
Support connection for the previous version of IceAdapter.
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.