Conversation
WalkthroughAdds a catch‑all listener API and wiring: new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Deserializer as EventDeserializer
participant Namespace
participant DataListeners as DataListeners
participant Interceptors as EventInterceptors
participant CatchAll as CatchAllListeners
Client->>Deserializer: Send event payload (namespace, event, data)
alt Event mapped in namespace
Deserializer->>Namespace: Event with mapped args
else No mapping for event
Deserializer->>Deserializer: Parse remaining tokens -> List<Object>
Deserializer->>Namespace: Event with generic args
end
Namespace->>DataListeners: Lookup EventEntry and invoke DataListeners (only if entry != null)
DataListeners-->>Namespace: Completed
Namespace->>Interceptors: Run EventInterceptors
Interceptors-->>Namespace: Completed
Namespace->>CatchAll: Invoke each CatchAllEventListener(client, event, args, ack)
CatchAll-->>Namespace: Completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java (1)
26-26: Consider explicit imports for better code clarity.While wildcard imports are convenient, explicit imports make dependencies clearer and help avoid naming conflicts. However, since this is an internal package and the change is consistent across the codebase, this is acceptable.
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java (1)
8-15: Add comprehensive JavaDoc for the new public API.The
CatchAllEventListenerinterface would benefit from detailed documentation covering:
- The purpose and use case for catch-all event listeners
- When
onEventis invoked (for all events, including unregistered ones)- Execution order relative to
EventInterceptorandDataListener- Thread safety guarantees (called from event processing thread)
- Parameter descriptions
- Usage example showing registration via
SocketIOServer.onAny()orNamespace.onAny()Example documentation structure:
/** * A catch-all event listener that receives all Socket.IO events for a namespace, * regardless of whether specific event listeners are registered. * <p> * Catch-all listeners are invoked after {@link DataListener}s and {@link EventInterceptor}s * in the following order: * <ol> * <li>Data listeners (if the event is registered)</li> * <li>Event interceptors</li> * <li>Catch-all listeners</li> * </ol> * <p> * This is useful for forwarding unknown events to downstream services or logging all events. * <p> * Thread Safety: Invoked from the event processing thread. Implementations should be thread-safe * if registered on multiple namespaces. * <p> * Example usage: * <pre>{@code * server.onAny((client, eventName, args, ackRequest) -> { * logger.info("Event received: {} from {}", eventName, client.getSessionId()); * }); * }</pre> * * @author https://github.com/sanjomo * @date 27/11/25 10:31 am */ @FunctionalInterface public interface CatchAllEventListener { /** * Called when any Socket.IO event is received. * * @param client the client that sent the event * @param eventName the name of the event * @param args the event arguments * @param ackRequest the acknowledgment request, can be used to send a response */ void onEvent(SocketIOClient client, String eventName, List<Object> args, AckRequest ackRequest); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(2 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (6)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java (1)
367-370: LGTM!The implementation correctly delegates to
mainNamespace.onAny(listener), consistent with the delegation pattern used for other listener methods in this class.netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (4)
32-32: Wildcard import is acceptable for internal package.Consistent with the change in
SocketIOServer.java, using wildcard import for the listener package is acceptable since it's internal.
71-71: LGTM!Using
ConcurrentLinkedQueuefor catch-all listeners is correct and consistent with the thread-safe pattern used for other listener queues in this class.
131-134: LGTM!The
onAnyimplementation correctly adds the listener to thecatchAllEventListenersqueue, consistent with the registration pattern used for other listener types.
158-184: LGTM! Catch-all listener invocation is correctly implemented.The modification correctly enables catch-all listeners to handle events regardless of whether specific listeners are registered, which fulfills the PR objective. Key aspects:
- Regular data listeners are processed only when an
EventEntryexists- Event interceptors and catch-all listeners are invoked for all events, including unregistered ones
- Execution order: data listeners → event interceptors → catch-all listeners → acknowledgment
The null-guarding on line 162 is essential for supporting the catch-all feature.
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java (1)
50-51: I'll help you verify this review comment. Let me search the codebase to understand the scope and impact of adding theonAny()method to theClientListenersinterface.
<function_calls>
#!/bin/bashFirst, let's locate and understand the ClientListeners interface
echo "=== ClientListeners interface definition ==="
find . -name "ClientListeners.java" -type f 2>/dev/nullSearch for the interface and its documentation
echo -e "\n=== ClientListeners interface content ==="
rg "interface ClientListeners" -A 20 --type javaLook for implementations of ClientListeners
echo -e "\n=== Implementations of ClientListeners ==="
rg "implements.*ClientListeners" --type java -B 2 -A 5Check if ClientListeners is mentioned in public API or documentation
echo -e "\n=== References to ClientListeners in codebase ==="
rg "ClientListeners" --type java -l | head -20
</function_calls>
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Fixed
Show fixed
Hide fixed
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
19-40: MissingExceptionListenerimport causes compilation failureThe
ExceptionListenertype is used at line 96 (private final ExceptionListener exceptionListener;) and line 101 (constructor), but the import statement is missing from the file. This will cause a "cannot find symbol: class ExceptionListener" compile error.Add the import:
import com.socketio4j.socketio.listener.EventInterceptor; +import com.socketio4j.socketio.listener.ExceptionListener; import com.socketio4j.socketio.listener.MultiTypeEventListener;
🧹 Nitpick comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/zTest.java (1)
9-30: Demo main class should live in tests/examples, not main library, and minor naming cleanupsFunctionally this is a good minimal example of
onAny+ specific listener usage. However:
- Having a
zTestmain class undersrc/main/javameans it will be packaged with the library; this is usually undesirable for a core server artifact. Consider moving this to a test (src/test/java) or a dedicated examples/demo module.- The variable name
fffin theonAnylambda is unclear; renaming it to something likeargswould make the example easier to follow.- Optional: the hard-coded port
9092is fine for a demo, but you might want to at least mention in a comment that it’s just an example/default.netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (2)
47-79: Catch‑all listener storage andonAnyAPI look consistent with existing listener patternsThe introduction of
CatchAllEventListener, thecatchAllEventListenersconcurrent queue, and theonAny(CatchAllEventListener listener)method aligns well with the existing listener infrastructure (connectListeners,eventInterceptors, etc.). UsingConcurrentLinkedQueuekeeps listener registration thread‑safe and iteration weakly consistent, which matches how the other listener queues behave here.From an API perspective, this is a reasonable way to surface namespace‑level catch‑all listeners.
If you expect a high number of catch‑all registrations over time, you might later consider adding a removal API (similar to
removeAllListenersfor named events), but it’s not strictly necessary to support the feature as requested.Also applies to: 138-141
165-191:onEventnow runs interceptors and catch‑all listeners even when no specific listener is registered—verify this behavior and add testsThe refactor to:
- Guard the
DataListenerloop withif (entry != null), and- Always run
eventInterceptorsand thencatchAllEventListenerinstances,means that for events without a registered
EventEntryyou will now:
- Still invoke
EventInterceptor.onEvent(...).- Invoke
CatchAllEventListener.onEvent(...).- Still follow the ACK flow via
sendAck(ackRequest)(subject toackModeand exceptions).This change appears intentional and correctly addresses the “onAny runs even when no specific listener exists” requirement, but it is also a subtle behavioral change compared to only processing events with a concrete
EventEntry.I recommend adding targeted tests that cover:
- Events with a specific listener (ensure existing behavior remains unchanged).
- Events without a specific listener but with an interceptor and/or catch‑all listener (ensure they now fire, and ACK behavior matches your intended contract).
- Failure paths (exceptions in a
DataListener,EventInterceptor, orCatchAllEventListener) to confirmAckMode.AUTO_SUCCESS_ONLYstill behaves as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(2 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(4 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/zTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
- netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java
🧰 Additional context used
🧬 Code graph analysis (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/zTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
Namespace(70-506)
🪛 GitHub Actions: Build PR
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java
[error] 95-95: cannot find symbol: class ExceptionListener
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
139-142: Consider adding a null check and JavaDoc.The method implementation is correct. For improved robustness and API clarity, consider:
- Adding a null check for the listener parameter
- Adding JavaDoc to document the behavior, especially that catch-all listeners execute after both regular event listeners and interceptors
+ /** + * Registers a catch-all event listener that will be invoked for every event, + * regardless of whether a specific listener is registered for that event. + * Catch-all listeners execute after regular event listeners and interceptors. + * + * @param listener the catch-all listener to register + */ @Override public void onAny(CatchAllEventListener listener) { + if (listener == null) { + throw new IllegalArgumentException("listener cannot be null"); + } catchAllEventListeners.add(listener); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (4)
79-79: LGTM!The use of
ConcurrentLinkedQueueis consistent with other listener collections in this class and provides appropriate thread-safety for concurrent listener registration and iteration.
170-183: LGTM! Critical logic correctly enables catch-all functionality.The modification to wrap data listener processing in
if (entry != null)is the key enabler for the catch-all feature. This ensures that interceptors and catch-all listeners execute even when no specific event listener is registered, fulfilling the PR objective.Execution order is well-designed:
- Specific event listeners (if registered)
- Event interceptors (always)
- Catch-all listeners (always)
Exception handling correctly covers all listener invocations, and the thread-safe iteration of
ConcurrentLinkedQueueis appropriate for concurrent scenarios.
47-47: I'll help you verify the review comment about theCatchAllEventListenerimport. Let me start by finding and inspecting the interface definition.
<function_calls>
#!/bin/bashFind the CatchAllEventListener interface file
fd -type f -name "CatchAllEventListener.java"
</function_calls>Let me also search for the interface definition within the listener package:
<function_calls>
#!/bin/bashSearch for CatchAllEventListener interface definition
rg -n "interface CatchAllEventListener" --type java
</function_calls>
139-142: Integration verified as correctly implemented.The catch-all listener registration flows properly through the API layers:
SocketIOServer.onAny(listener)correctly delegates tomainNamespace.onAny(listener)(line 377 in SocketIOServer.java)mainNamespaceis typed asSocketIONamespaceinterface and instantiated as aNamespaceobjectClientListenersinterface defines theonAnymethod contractNamespace.onAny()implementation correctly registers listeners in thecatchAllEventListenerscollection- Consistent delegation pattern is used across all listener registration methods
No integration issues found.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
This reverts commit dc21393.
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Fixed
Show fixed
Hide fixed
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/JacksonJsonSupport.java (1)
183-200: Generic-args fallback for unmapped events looks correct; add tests to lock behavior inThe new branch that, when no mapping exists for either the current namespace or
DEFAULT_NAME, parses remaining array elements intoList<Object>and returnsnew Event(eventName, genericArgs)is consistent with the catch‑all use case and appears token-safe.Recommend adding focused tests for:
- unmapped events with 0, 1, and multiple args, and
- events with nested objects/arrays,
to prevent regressions in this fallback path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(2 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(4 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/JacksonJsonSupport.java(1 hunks)
🔇 Additional comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java (1)
29-29: Server‑level catch‑all registration is wired correctly
onAnyEventand itsonAnyalias delegate straight tomainNamespace, which keeps the server API thin and consistent with existing listener wiring. No issues spotted.Also applies to: 375-383
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Show resolved
Hide resolved
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java
Show resolved
Hide resolved
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
139-147: Consider adding a removal API for catch-all listeners.The registration methods are straightforward and consistent with Socket.IO conventions (providing both
onAnyEventand theonAnyalias). However, without a corresponding removal method (e.g.,offAnyorremoveOnAnyEventListener), dynamically registered catch-all listeners cannot be unregistered, potentially leading to memory accumulation in long-running applications.If dynamic registration/removal is expected, consider adding:
@Override public void offAny(CatchAllEventListener listener) { catchAllEventListeners.remove(listener); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
🔇 Additional comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java (1)
1-16: LGTM! Functional interface signature is correct.The interface correctly defines the catch-all event listener contract with all four parameters (client, eventName, args, ackRequest). This signature matches the invocation in
Namespace.onEvent(line 186) and aligns with the existingEventInterceptorpattern. The parameters are part of the listener contract even if some implementations choose to ignore certain parameters.netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (2)
79-79: LGTM! Appropriate data structure for concurrent listener storage.Using
ConcurrentLinkedQueueis consistent with other listener queues in this class and provides thread-safe, non-blocking operations suitable for the event-driven architecture.
174-187: LGTM! Event handling flow correctly implements catch-all listener support.The changes successfully address issue #58 by:
- Making specific listener invocation conditional (
if (entry != null)), preventing NPEs for unregistered events- Ensuring event interceptors run regardless of specific listener registration
- Invoking catch-all listeners after interceptors, providing the desired "onAny" capability
The execution order (specific listeners → interceptors → catch-all listeners) is logical, and the signature at line 186 correctly matches the
CatchAllEventListenerinterface. Exception handling appropriately wraps all listener types.
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Fixed
Show fixed
Hide fixed
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java (1)
49-53: Abstract method on public interface remains a breaking change.The past review correctly identified that adding
onAnyEventas an abstract method breaks existing implementations ofClientListeners. The PR claims to be non-breaking, but this change would cause compile errors for any external code implementing this interface.The previous suggestion to use a default method has not been applied.
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
141-162: Add null validation and consider a removal API.
Null check: Passing a null listener would silently add it to the queue, causing an NPE during event dispatch.
Removal API: Without an
offAny/removeOnAnyEventListenermethod, callers cannot unregister catch-all listeners. This can cause memory leaks or unintended behavior in scenarios with dynamic listener lifecycle management.@Override public void onAnyEvent(CatchAllEventListener listener) { + if (listener == null) { + throw new IllegalArgumentException("listener cannot be null"); + } catchAllEventListeners.add(listener); } + + public void offAny(CatchAllEventListener listener) { + catchAllEventListeners.remove(listener); + }Apply similar null checks to the other
onAnyoverloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(2 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java
- netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (3)
31-32: LGTM!The imports are appropriately added for the new functional interfaces (
BiConsumer,Consumer) and theCatchAllEventListener.Also applies to: 49-49
81-81: LGTM!Using
ConcurrentLinkedQueueis consistent with how other listener collections are managed in this class and provides appropriate thread-safety.
186-212: LGTM!The event processing flow correctly implements the catch-all listener requirement:
DataListeners are only invoked when a specific listener exists (entry != null), preventing NPE for unknown events.EventInterceptors andCatchAllEventListeners are always invoked regardless of whether a specific listener is registered, which fulfills the objective from issue #58.The ordering (specific listeners → interceptors → catch-all) is reasonable, and all listeners are wrapped in the existing exception handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java (1)
51-53: Add javadoc to document the new public API.The
onAnyEventmethod is a new public API but lacks documentation. Adding javadoc will help users understand its purpose, the role ofCatchAllEventListener, and why the default throwsUnsupportedOperationException.Consider adding documentation similar to this:
+/** + * Registers a catch-all listener that will be invoked for every event received by this client, + * regardless of whether a specific event listener is registered. + * <p> + * This method provides a default implementation that throws {@link UnsupportedOperationException}. + * Concrete implementations must override this method to provide actual catch-all listener support. + * + * @param listener the catch-all event listener to register + * @throws UnsupportedOperationException if the implementation does not support catch-all listeners + * @see CatchAllEventListener + */ default void onAnyEvent(CatchAllEventListener listener) { throw new UnsupportedOperationException("onAnyEvent is not implemented"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java (1)
51-53: Verified: Both dependent implementations properly overrideonAnyEventwith actual functionality.The verification confirms the review comment's concern is valid and the implementations are correct:
- SocketIOServer (line 376-378): Overrides
onAnyEventand delegates tomainNamespace.onAnyEvent(listener)- Namespace (line 142-144): Overrides
onAnyEventand adds the listener tocatchAllEventListenerscollectionBoth implementations provide actual catch-all listener functionality instead of throwing the
UnsupportedOperationExceptionfrom the default method. The inheritance chain is:Namespace→SocketIONamespace(which extendsClientListeners) →ClientListeners.
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
31-33: Event flow + catch‑all integration meet goals; consider making BiConsumer/Consumer listeners removablePositives:
catchAllEventListenersbacked byConcurrentLinkedQueue(Lines 79-82) is appropriate for concurrent registration and iteration.onEventnow checksentry != nullbefore iterating listeners (Lines 199-205), avoiding NPEs on unknown events.eventInterceptorsare always invoked, even when there is no specificEventEntry(Lines 208-210), matching the requirement that interceptors run for all events.- Catch‑all listeners are invoked for every event after interceptors (Lines 211-213), with full context (
client,eventName,args,ackRequest), which is exactly what a server‑sideonAnyneeds.One design improvement to consider:
- The convenience overloads
onAny(BiConsumer<String, List<Object>>)andonAny(Consumer<String>)(Lines 162-172) wrap user callbacks into anonymousCatchAllEventListenerinstances stored internally. Because those instances are not exposed, it’s hard for callers to later remove them viaremoveOnAnyEventListener/offAny, which can lead to listener buildup over time.A simple pattern to keep the convenience API while enabling removal would be to return the created listener so callers can hold onto it:
- public void onAny(BiConsumer<String, List<Object>> listener) { - catchAllEventListeners.add((client, event, args, ack) -> - listener.accept(event, args) - ); - } + public CatchAllEventListener onAny(BiConsumer<String, List<Object>> listener) { + CatchAllEventListener l = + (client, event, args, ack) -> listener.accept(event, args); + catchAllEventListeners.add(l); + return l; + }(and similarly for the
Consumer<String>overload).That keeps the sugar methods while allowing callers to later pass the same
CatchAllEventListenerinstance toremoveOnAnyEventListener/offAnyto avoid leaks.Also applies to: 49-49, 79-82, 141-172, 195-213
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(2 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/EventInterceptor.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/EventInterceptor.java (1)
24-25: Whitespace-only touch; interface contract unchangedLine 24 just normalizes spacing in the interface declaration; signature and behavior remain identical, so no risk here.
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java (1)
51-57: Non-breaking SPI extension for catch‑all listeners looks goodMaking
addOnAnyEventListener/removeOnAnyEventListenerdefault methods that throwUnsupportedOperationExceptionpreserves compatibility for existingClientListenersimplementors while clearly signaling optional support for the new feature. This aligns well with the stated “non‑breaking” goal.
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (1)
164-174: Consider exposing a removable handle for functional onAny() overloadsThe
onAny(BiConsumer<String, List<Object>>)andonAny(Consumer<String>)helpers are convenient, but since they internally wrap the passed functional value into a newCatchAllEventListener, there’s no straightforward way for callers to later unregister these viaoffAnywithout holding that wrapper themselves.You might want to either:
- Return the created
CatchAllEventListenerfrom these overloads so callers can pass it tooffAny, or- Add corresponding
offAny(BiConsumer<String, List<Object>>)/offAny(Consumer<String>)overloads that track and remove the underlying wrapper.This would bring the ergonomic parity of the functional helpers closer to the core
CatchAllEventListenerAPI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(2 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java (2)
29-29: CatchAllEventListener import matches new listener API usageThe new import is correct and scoped only to support the onAny/offAny delegation methods added below; no issues here.
375-395: Server-level onAny/offAny delegation is consistent and correctThe add/remove/onAny/offAny methods cleanly delegate to
mainNamespace, mirroring how other listener APIs are exposed at the server level.offAnycorrectly callsremoveOnAnyEventListener, so catch‑all listeners can be unregistered as expected.netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java (2)
31-32: Catch-all listener storage and core onAny/offAny API look solidUsing a
ConcurrentLinkedQueue<CatchAllEventListener>plusaddOnAnyEventListener/removeOnAnyEventListenerand theonAny/offAnyaliases is consistent with the rest of the listener infrastructure and thread-safe for concurrent registration and dispatch. The API surface aligns well with the new server-level delegation.Also applies to: 49-49, 81-81, 141-162
201-215: onEvent flow correctly preserves interceptors and catch-all behavior for unknown eventsGuarding the
DataListenerloop withif (entry != null)avoids NPEs for events without registered listeners, whileeventInterceptorsandcatchAllEventListenersare still invoked for every event. This matches the catch‑all/interceptor objectives and maintains existing ack behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/JacksonJsonSupport.java (1)
183-199: Generic fallback for unmapped events looks sound; consider observability/docsThe new branch that checks
!eventMapping.containsKey(ek)for both the current namespace andNamespace.DEFAULT_NAMEand then parses remaining tokens asObjectintogenericArgsis logically correct and matches the catch‑all/onAny goal: unknown events no longer lose their payloads, and the parser state stays consistent by walking toEND_ARRAY.Two optional improvements you might consider:
- Add a low‑level
debug(or maybeinfogated by config) log when this generic fallback is taken, includingnamespaceClass.get()andeventName, so operators can see when events are arriving without explicit mappings.- Document this behavior (e.g., Javadoc on
JsonSupport/JacksonJsonSupportor server docs) so users know that unmapped events produceList<Object>args using Jackson’s default typing, which is what onAny/catch‑all handlers will see.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/JacksonJsonSupport.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: build (25) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Dismissed
Show dismissed
Hide dismissed
netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/CatchAllEventListener.java
Dismissed
Show dismissed
Hide dismissed
|
@NeatGuyCoding Please review |
|
Description
Brief description of the changes in this PR.
Type of Change
Related Issue
Closes #58
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.