Redis Stream support - Single channel Hazelcast & Redis - Distributed Test #77
Conversation
commit dad09c1 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 21:47:22 2025 +0530 checkstyle issues commit c04e9c4 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 20:15:10 2025 +0530 stream logic change by reliable topic Redisson reliable topic -> stream Hazalcast reliable topic -> ringbuffer commit 018d94a Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 13:03:52 2025 +0530 review comment changes commit 614ecad Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 12:27:08 2025 +0530 Update ListenerRegistration.java commit 313e6c9 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 12:14:14 2025 +0530 review changes commit a339591 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 11:47:53 2025 +0530 review changes commit b9bfb15 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 01:12:00 2025 +0530 typo fix commit eba8962 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 13 00:33:46 2025 +0530 Refactor RedisStreamsStore and improve test reliability Refactored RedisStreamsStore to improve executor management, use JetBrains @nullable annotations, and enhance shutdown handling. Updated tests to remove unnecessary Thread.sleep calls and ensure proper resource cleanup, improving test reliability and speed. Added org.jetbrains.annotations as a dependency and updated module-info.java accordingly. commit d0839dc Author: sanjomo <git@santhoshmohan.com> Date: Fri Dec 12 13:48:28 2025 +0530 review changes and test refactoring commit c8780e5 Author: sanjomo <git@santhoshmohan.com> Date: Fri Dec 12 12:54:59 2025 +0530 code refactoring removed retry mechanism, messages only once processed commit e74792e Author: sanjomo <git@santhoshmohan.com> Date: Fri Dec 12 02:31:36 2025 +0530 thread for each channel commit c82ed3a Author: sanjomo <git@santhoshmohan.com> Date: Fri Dec 12 01:29:29 2025 +0530 checkstyle changes commit db96284 Author: sanjomo <git@santhoshmohan.com> Date: Fri Dec 12 00:52:39 2025 +0530 test refactoring commit fb705a0 Author: sanjomo <git@santhoshmohan.com> Date: Fri Dec 12 00:14:26 2025 +0530 test refactoring commit 36f53f3 Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 23:27:26 2025 +0530 checkstyle issue fix commit 269c9cf Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 22:49:41 2025 +0530 test refactoring commit 68e9ecc Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 21:57:32 2025 +0530 license added commit 8c9d122 Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 21:31:45 2025 +0530 Update RedisStreamsStoreFactory.java commit 0f75adf Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 21:18:52 2025 +0530 code refactoring commit 92f9cea Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 19:11:36 2025 +0530 code refactoring commit 3db6cc4 Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 16:20:15 2025 +0530 Update DefaultSocketIOBeans.java commit ba8e4fd Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 16:12:40 2025 +0530 Delete DevMain.java commit c79a9ab Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 16:06:00 2025 +0530 test cases distributed cases commit 2683aed Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 13:59:04 2025 +0530 remove getEnabledTypes as all should be enabled commit 6cc4694 Merge: 05c3c02 2d5c28f Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 13:58:05 2025 +0530 Merge branch 'redis-stream-single-channel' of https://github.com/socketio4j/netty-socketio into redis-stream-single-channel commit 05c3c02 Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 11 13:55:08 2025 +0530 redis stream multichannel support commit 2d5c28f Author: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com> Date: Thu Dec 11 10:45:28 2025 +0800 refactor(minor): add log for event store exception Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com> commit 5ccb56c Author: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com> Date: Thu Dec 11 09:43:04 2025 +0800 refactor(minor): add log for event store throwable Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com> commit d523d14 Author: sanjomo <git@santhoshmohan.com> Date: Tue Dec 9 21:58:57 2025 +0530 checkstyle change commit 931ba68 Author: sanjomo <git@santhoshmohan.com> Date: Tue Dec 9 21:30:49 2025 +0530 code refactoring commit a555ac4 Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 6 20:24:31 2025 +0530 checkstyle changes commit 7fd2ffd Author: sanjomo <git@santhoshmohan.com> Date: Sat Dec 6 20:09:48 2025 +0530 adding different store type commit 1068397 Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 4 10:44:55 2025 +0530 checkstyle changes commit b5ffdab Author: sanjomo <git@santhoshmohan.com> Date: Thu Dec 4 10:28:10 2025 +0530 test case optimisations commit 53cb1d3 Author: sanjomo <git@santhoshmohan.com> Date: Wed Dec 3 23:10:35 2025 +0530 boardcast and redissonpub sub test commit 82bb552 Author: sanjomo <git@santhoshmohan.com> Date: Wed Dec 3 20:23:18 2025 +0530 code review changes commit 484fd28 Author: sanjomo <git@santhoshmohan.com> Date: Wed Dec 3 19:41:32 2025 +0530 Update DistributedSingleChannelRedisStreamTest.java commit 983ed11 Author: sanjomo <git@santhoshmohan.com> Date: Wed Dec 3 18:37:46 2025 +0530 test case added, pubsubstore to eventstore name change commit 6abd6a0 Author: sanjomo <git@santhoshmohan.com> Date: Wed Dec 3 16:05:47 2025 +0530 gitignore autogenerated xml commit 5fc76c8 Author: sanjomo <git@santhoshmohan.com> Date: Wed Dec 3 15:45:39 2025 +0530 nodeid logic change, constructors stores, other refactoring commit f3fb2c2 Author: sanjomo <git@santhoshmohan.com> Date: Tue Dec 2 23:07:11 2025 +0530 license add commit 5fc4af3 Author: sanjomo <git@santhoshmohan.com> Date: Tue Dec 2 22:54:16 2025 +0530 checkstyle changes commit 2affd53 Author: sanjomo <git@santhoshmohan.com> Date: Tue Dec 2 22:33:12 2025 +0530 review changes commit ea23a72 Merge: 6aa868a 9945958 Author: sanjomo <git@santhoshmohan.com> Date: Tue Dec 2 22:21:13 2025 +0530 Merge branch 'main' into redis-stream-single-channel commit 6aa868a Author: sanjomo <git@santhoshmohan.com> Date: Sat Nov 29 15:37:48 2025 +0530 pubsub to event commit 768ffd4 Author: sanjomo <git@santhoshmohan.com> Date: Fri Nov 28 20:11:00 2025 +0530 Update SingleChannelRedisStreamsPubSubStore.java commit 581225f Author: sanjomo <git@santhoshmohan.com> Date: Wed Nov 26 20:45:21 2025 +0530 redis stream single channel - order guaranteed
WalkthroughThis PR replaces the pub/sub store architecture with a new event store abstraction. Changes include relocating store implementations to new packages, introducing EventStore/EventType/EventMessage/EventListener interfaces, removing HazelcastPubSubStore and BaseStoreFactory from the pubsub package, and adding new event-based store implementations (HazelcastEventStore, RedissonEventStore, RedissonReliableEventStore, MemoryEventStore) with corresponding factory classes and integration tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
...cketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java
Fixed
Show fixed
Hide fixed
...etio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
Fixed
Show fixed
Hide fixed
...etio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
Fixed
Show fixed
Hide fixed
...ore/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java
Fixed
Show fixed
Hide fixed
| UUID sessionId = UUID.randomUUID(); | ||
| Store store = storeFactory.createStore(sessionId); | ||
| PubSubStore pubSubStore = storeFactory.pubSubStore(); | ||
| EventStore eventStore = storeFactory.eventStore(); |
Qodana for JVM45 new problems were found
☁️ View the detailed Qodana report Detected 123 dependenciesThird-party software listThis page lists the third-party software dependencies used in project
Contact Qodana teamContact us at qodana-support@jetbrains.com
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major 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 (5)
pom.xml (1)
562-580: Fix incorrect module name in--add-opensand consider gating test retries via CI profile.The
--add-opens netty.socketio/com.socketio4j.socketio.store.pubsub(lines 569, 571) references a non-existent package; the actual package iscom.socketio4j.socketio.store.redis_pubsub(which is already exported in module-info). Either update these flags to use the correct package name or remove them if no longer needed.Also,
rerunFailingTestsCount=3is unconditional and applies to all test runs. Test retries can mask flakiness. Consider gating this via a CI profile and reporting when retries occur, so real nondeterminism surfaces rather than silently passing.netty-socketio-core/src/test/java/com/socketio4j/socketio/store/AbstractStoreTest.java (1)
43-64: Container lifecycle changed from per-class to per-test without explicit cleanup.The
containerfield was changed from static to instance, removing the@AfterAllcleanup. While the container starts in@BeforeEach, there's no explicit stop in@AfterEach. This could lead to:
- Resource leaks if containers accumulate without cleanup
- Slower test execution due to repeated container creation/destruction
Consider either:
- Restoring the static container for shared lifecycle across tests, or
- Adding explicit container cleanup in
@AfterEach:@AfterEach public void tearDown() throws Exception { if (store != null) { // Clean up store data cleanupStore(); } + if (container != null && container.isRunning()) { + container.stop(); + } }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/memory/MemoryStoreFactory.java (1)
27-43: CallmemoryEventStore.shutdown()in the factory's shutdown method.
All otherStoreFactoryimplementations (Hazelcast, Redis PubSub, Redis Stream) cascade the shutdown call to theireventStore. For consistency and to prepare for future resource management inMemoryEventStore, add the same pattern here.@Override public void shutdown() { + memoryEventStore.shutdown(); }(The
getEventStoreMode()check inBaseStoreFactory.init(...)is safe;MemoryEventStorereturnsEventStoreMode.SINGLE_CHANNEL.)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
71-82: Same issue:publishConfigparameter is unused.This constructor also accepts but ignores
publishConfig. Consider refactoring both constructors to either use the config or remove the parameter.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
63-73:publishConfigparameter is accepted but never used.Similar to
RedissonStoreFactory, this constructor acceptsPublishConfig publishConfigbut never passes it toHazelcastEventStore. This inconsistency could mislead API consumers who expect their publish configuration to be applied.
♻️ Duplicate comments (3)
netty-socketio-quarkus/netty-socketio-quarkus-runtime/src/main/java/com/socketio4j/socketio/quarkus/recorder/NettySocketIOConfigRecorder.java (3)
146-151: Same bean lifecycle issue as ExceptionListener.This block has the same critical issue with try-with-resources destroying dependent-scoped beans while retaining references. Apply the same fix as suggested for the ExceptionListener block.
153-158: Same bean lifecycle issue as ExceptionListener.This block has the same critical issue with try-with-resources destroying dependent-scoped beans while retaining references. Apply the same fix as suggested for the ExceptionListener block.
160-165: Same bean lifecycle issue as ExceptionListener.This block has the same critical issue with try-with-resources destroying dependent-scoped beans while retaining references. Apply the same fix as suggested for the ExceptionListener block.
🟡 Minor comments (12)
netty-socketio-core/src/main/java/module-info.java-13-19 (1)
13-19: Changeorg.jetbrains.annotationstorequires static.The JetBrains annotations package should use
requires static org.jetbrains.annotationsrather thanrequires, since these annotations (@NotNull,@Nullable, etc.) are compile-time only and should not be a runtime dependency.Additionally, lines 16-19 and line 37 use 4-space indentation instead of the 2-space indentation used elsewhere in the file; consider aligning the formatting.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java-39-49 (1)
39-49: AvoidfindAvailablePort()TOCTOU race by binding the probe socket to loopback.
new ServerSocket(0)then releasing the port creates a race condition with other processes/tests acquiring the port. Since test servers connect tolocalhost, bind the probe socket to loopback to reduce the attack surface:-import java.net.ServerSocket; +import java.net.InetAddress; +import java.net.ServerSocket; @@ private int findAvailablePort() throws Exception { - try (ServerSocket socket = new ServerSocket(0)) { + try (ServerSocket socket = new ServerSocket(0, 0, InetAddress.getLoopbackAddress())) { return socket.getLocalPort(); } }Note:
Configuration.setPort(0)exists butSocketIOServerdoes not expose the bound port post-start via public API, so the loopback probe approach is the practical solution.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java-55-57 (1)
55-57: Fix misleading comment - this is Hazelcast, not Redis.The comment says "Redis + Node Setup" but this test uses Hazelcast. This appears to be a copy-paste error from a Redis-based test.
// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------netty-socketio-core/src/test/java/com/socketio4j/socketio/store/StoreFactoryTest.java-85-91 (1)
85-91: Fix stale assertion message referencing PubSubStore.The assertion message on line 90 still references "PubSubStore interface" but should say "EventStore interface" to match the updated test.
@Test public void testCreateEventStore() { EventStore eventStore = storeFactory.eventStore(); assertNotNull(eventStore, "EventStore should not be null"); - assertInstanceOf(EventStore.class, eventStore, "EventStore should implement PubSubStore interface"); + assertInstanceOf(EventStore.class, eventStore, "EventStore should implement EventStore interface"); }netty-socketio-core/src/test/java/com/socketio4j/socketio/store/MemoryStoreFactoryTest.java-63-69 (1)
63-69: Fix stale assertion messages referencing PubSubStore.The assertion messages on lines 67-68 still reference the old
PubSubStore/MemoryPubSubStorenames but should referenceEventStore/MemoryEventStore.@Test public void testMemoryEventStore() { EventStore eventStore = storeFactory.eventStore(); - assertNotNull(eventStore, "PubSubStore should not be null"); - assertTrue(eventStore instanceof MemoryEventStore, "PubSubStore should be MemoryPubSubStore"); + assertNotNull(eventStore, "EventStore should not be null"); + assertTrue(eventStore instanceof MemoryEventStore, "EventStore should be MemoryEventStore"); }netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonStreamEventStoreTest.java-28-30 (1)
28-30: Class name doesn't match the implementation being tested.The class is named
RedissonStreamEventStoreTest, suggesting it tests Redis Streams functionality, but it instantiatesRedissonEventStorefrom theredis_pubsubpackage. Based on the relevant code snippet,RedissonEventStoreusesRTopic(Redis Pub/Sub), notRStream(Redis Streams).If this test is intended for Redis Pub/Sub, rename it to
RedissonPubSubEventStoreTest. If it should test Redis Streams, it should use the appropriate Stream-based EventStore implementation (e.g., fromredis_streampackage).-public class RedissonStreamEventStoreTest extends AbstractEventStoreTest { +public class RedissonPubSubEventStoreTest extends AbstractEventStoreTest {Also applies to: 41-49
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStreamStoreFactoryTest.java-104-110 (1)
104-110: Update stale assertion messages from PubSubStore to EventStore.The assertion messages still reference the old "PubSubStore" naming convention but the test validates
EventStoreandRedissonEventStore. This is inconsistent with the migration to EventStore.@Test public void testRedissonEventStore() { EventStore eventStore = storeFactory.eventStore(); - assertNotNull(eventStore, "PubSubStore should not be null"); - assertTrue(eventStore instanceof RedissonEventStore, "PubSubStore should be RedissonPubSubStore"); + assertNotNull(eventStore, "EventStore should not be null"); + assertTrue(eventStore instanceof RedissonEventStore, "EventStore should be RedissonEventStore"); }netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastRingBufferStoreFactoryTest.java-110-114 (1)
110-114: Fix misleading assertion message.The assertion message states "EventStore should be HazelcastStore" but the actual check verifies
HazelcastEventStore.class.public void testHazelcastEventStore() { EventStore eventStore = storeFactory.eventStore(); assertNotNull(eventStore, "EventStore should not be null"); - assertInstanceOf(HazelcastEventStore.class, eventStore, "EventStore should be HazelcastStore"); + assertInstanceOf(HazelcastEventStore.class, eventStore, "EventStore should be HazelcastEventStore"); }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java-73-84 (1)
73-84: Remove unusedpublishConfigparameter.The
publishConfigparameter is accepted but never used in this constructor. It's not passed toRedissonStreamEventStoreor stored anywhere. This pattern is consistent across all store factory implementations (Redisson, RedissonStream, and Hazelcast), suggesting the parameter should either be removed from the signature or the event store implementations should be extended to support it.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java-134-139 (1)
134-139:shutdown0callsunsubscribeinstead ofunsubscribe0, causing redundant error logging.The
unsubscribemethod inEventStorewrapsunsubscribe0with try-catch logging. Callingthis::unsubscribefromshutdown0will result in errors being logged twice (once here if re-thrown, once in the wrapper).@Override public void shutdown0() { - Arrays.stream(EventType.values()).forEach(this::unsubscribe); + Arrays.stream(EventType.values()).forEach(this::unsubscribe0); map.clear(); //do not shut down client here }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java-255-261 (1)
255-261: Redundantshutdown()followed byshutdownNow().Calling
trimExecutor.shutdown()at line 257 and thentrimExecutor.shutdownNow()at line 260 is redundant. TheshutdownNow()call immediately interrupts tasks, negating the orderly shutdown initiated byshutdown().Either use
shutdown()withawaitTermination()for graceful shutdown, or useshutdownNow()alone for immediate termination:@Override public void shutdown0() { - trimExecutor.shutdown(); Arrays.stream(EventType.values()).forEach(this::unsubscribe); map.clear(); trimExecutor.shutdownNow(); }netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java-506-516 (1)
506-516: Document the ID matching assumption from Socket.IO protocol.The comparison assumes
a.id()(from the Socket.IO client) matches the server-sidegetSessionId(). This is guaranteed by the Socket.IO protocol—the server sends its session ID to the client in the CONNECT packet, which the client stores asid(). However, clarify this assumption in a comment to make the code's intent explicit.
🧹 Nitpick comments (34)
netty-socketio-core/src/main/java/com/socketio4j/socketio/JsonSupportWrapper.java (1)
47-47: Consider defensive wrapping ofsrc.readLine()in catch blocks.The
src.readLine()calls in the logging statements could potentially throwIOException, which would mask the original exception. While this is a pre-existing pattern, wrapping these calls in a try-catch would ensure the original diagnostic context is always logged.Example for line 47:
} catch (Exception e) { src.reset(); - log.error("Can't read ack args: {} for type: {}", src.readLine(), callback.getResultClass(), e); + String line = "unknown"; + try { + line = src.readLine(); + } catch (IOException readErr) { + // Ignore read failure during error logging + } + log.error("Can't read ack args: {} for type: {}", line, callback.getResultClass(), e); throw new IOException(e); }Also applies to: 58-58
netty-socketio-core/src/main/java/module-info.java (2)
20-20: Fix indentation for consistency.Line 20 has 4-space indentation instead of 2, inconsistent with the rest of the file.
37-37: Consider usingstatic requiresfor compile-time annotations.The
org.jetbrains.annotationsmodule provides compile-time-only annotations (CLASS retention) like@NotNulland@Nullablethat are not needed at runtime. Declare this asstatic requires org.jetbrains.annotations;to align with JPMS semantics and ensure it is treated as an optional dependency. Verify the corresponding build configuration usescompileOnly(Gradle) orprovidedscope (Maven).Also note the indentation inconsistency (4 spaces instead of 2).
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStore.java (1)
17-27: Package move looks consistent; consider containing the unchecked cast warning inget().
IfStore#getis intentionally generic-over-Object, add a local@SuppressWarnings("unchecked")to avoid propagating compiler noise and make the tradeoff explicit.@Override + @SuppressWarnings("unchecked") public <T> T get(String key) { return (T) map.get(key); }Also applies to: 40-43
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventStoreMode.java (1)
17-22:EventStoreModeenum is clean and self-explanatory.
Optional: add a short Javadoc on when SINGLE vs MULTI is expected to be used.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/StoreFactory.java (1)
26-36: Consider a deprecated compatibility shim forpubSubStore()to ease downstream migration.
Since this is a breaking API, a default method can reduce immediate churn for integrators while still steering them toeventStore().public interface StoreFactory extends Disconnectable { EventStore eventStore(); + + /** + * @deprecated Use {@link #eventStore()}. + */ + @Deprecated + default EventStore pubSubStore() { + return eventStore(); + } <K, V> Map<K, V> createMap(String name);netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BulkLeaveMessage.java (1)
22-50: MakeBulkLeaveMessagesafer for cross-thread / serialization use (immutability + defensive copies + null checks).
Right now callers can mutateroomsafter construction (or via the getter), and nulls can slip through until later.package com.socketio4j.socketio.store.event; +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; import java.util.Set; import java.util.UUID; public class BulkLeaveMessage extends EventMessage { private static final long serialVersionUID = 4084111480148299012L; - private UUID sessionId; - private String namespace; - private Set<String> rooms; + private UUID sessionId; + private String namespace; + private Set<String> rooms; public BulkLeaveMessage() { } - public BulkLeaveMessage(UUID id, Set<String> rooms, String namespace) { + public BulkLeaveMessage(UUID sessionId, Set<String> rooms, String namespace) { super(); - this.sessionId = id; - this.rooms = rooms; - this.namespace = namespace; + this.sessionId = Objects.requireNonNull(sessionId, "sessionId must not be null"); + this.namespace = Objects.requireNonNull(namespace, "namespace must not be null"); + Objects.requireNonNull(rooms, "rooms must not be null"); + this.rooms = Collections.unmodifiableSet(new HashSet<>(rooms)); } @@ public Set<String> getRooms() { return rooms; } }netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (1)
39-39:withReuse(true)+ explicitstop()is potentially contradictory—decide one behavior.
If you want deterministic cleanup in CI, drop reuse. If you want reuse locally, consider not stopping (or gate reuse with an env/property).Also applies to: 111-129
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/ListenerRegistration.java (1)
25-42: Encapsulate fields (private final) since this is a public API type.
Even though getters exist, package-private fields allow same-package mutation/reflection patterns you probably don’t want long-term.public final class ListenerRegistration<T extends EventMessage> { - final EventListener<T> listener; - final Class<T> clazz; + private final EventListener<T> listener; + private final Class<T> clazz;netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
53-102: Consider removing or replacing the commented-out sleep.The setup logic is correct and follows the established pattern for distributed tests. However, the commented-out
Thread.sleep(600)at line 101 suggests there may have been timing concerns during development.If synchronization is needed for nodes to be ready, consider using a proper readiness check (e.g., Awaitility) rather than a fixed sleep. If the sleep is no longer needed, remove the commented line to keep the code clean.
port2 = cfg2.getPort(); - - //Thread.sleep(600); }netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
36-108: Consider reducing duplication with multi-channel test.This test is nearly identical to
DistributedRedissonStreamMultiChannelTest, differing only inEventStoreMode.SINGLE_CHANNELvsMULTI_CHANNEL. This pattern is repeated across multiple test classes in this PR.Consider using JUnit 5's
@ParameterizedTestwith@EnumSourceto test both channel modes in a single class, or extract common setup logic to a shared base class or utility:// Example: Parameterized approach @ParameterizedTest @EnumSource(EventStoreMode.class) void testDistributedRedissonStream(EventStoreMode mode) throws Exception { // Setup with the given mode cfg1.setStoreFactory(new RedissonStreamStoreFactory(redisClient1, mode)); // ... rest of test }This would significantly reduce maintenance burden as the number of store types and modes grows.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java (2)
24-25: Remove unused imports.
Redisson,Config, andRedissonStoreFactoryare imported but never used in this Hazelcast-only test class.-import org.redisson.Redisson; -import org.redisson.config.Config; import com.hazelcast.client.HazelcastClient; import com.hazelcast.client.config.ClientConfig; @@ -33,7 +31,6 @@ import com.socketio4j.socketio.store.CustomizedHazelcastContainer; import com.socketio4j.socketio.store.event.EventStoreMode; import com.socketio4j.socketio.store.event.PublishConfig; import com.socketio4j.socketio.store.hazelcast.HazelcastStoreFactory; -import com.socketio4j.socketio.store.redis_pubsub.RedissonStoreFactory;Also applies to: 36-36
54-56: Fix misleading comment.The comment says "Redis + Node Setup" but this test sets up Hazelcast nodes.
// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (2)
24-26: Remove unused imports.
Redisson,RedissonClient,Config,CustomizedRedisContainer, andRedissonStreamStoreFactoryare imported but never used in this Hazelcast-only test class.-import org.redisson.Redisson; -import org.redisson.api.RedissonClient; -import org.redisson.config.Config; import com.hazelcast.client.HazelcastClient; import com.hazelcast.client.config.ClientConfig; @@ -31,9 +28,7 @@ import com.socketio4j.socketio.Configuration; import com.socketio4j.socketio.SocketIOServer; import com.socketio4j.socketio.store.CustomizedHazelcastContainer; -import com.socketio4j.socketio.store.CustomizedRedisContainer; import com.socketio4j.socketio.store.event.EventStoreMode; import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferStoreFactory; -import com.socketio4j.socketio.store.redis_stream.RedissonStreamStoreFactory;Also applies to: 34-34, 37-37
55-57: Fix misleading comment.The comment says "Redis + Node Setup" but this test sets up Hazelcast RingBuffer nodes.
// ------------------------------------------- - // Redis + Node Setup + // Hazelcast RingBuffer + Node Setup // -------------------------------------------netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/PublishConfig.java (1)
52-66: Consider adding factory helper with explicit default mode.The factory helpers are convenient. For completeness, you might consider adding a static factory that accepts the default mode directly for custom configurations.
public static PublishConfig withDefault(PublishMode defaultMode) { return new PublishConfig(defaultMode, Collections.emptyMap()); } public static PublishConfig withDefault(PublishMode defaultMode, Map<EventType, PublishMode> overrides) { return new PublishConfig(defaultMode, overrides); }netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (2)
24-36: Remove unused Redis-related imports.These imports reference Redis/Redisson classes that are not used in this Hazelcast-focused test. They appear to be copy-paste artifacts.
import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.TestInstance; -import org.redisson.config.Config; import com.hazelcast.client.HazelcastClient; import com.hazelcast.client.config.ClientConfig; import com.hazelcast.core.HazelcastInstance; import com.socketio4j.socketio.Configuration; import com.socketio4j.socketio.SocketIOServer; import com.socketio4j.socketio.store.CustomizedHazelcastContainer; -import com.socketio4j.socketio.store.CustomizedRedisContainer; import com.socketio4j.socketio.store.event.EventStoreMode; import com.socketio4j.socketio.store.event.PublishConfig; import com.socketio4j.socketio.store.hazelcast.HazelcastStoreFactory; -import com.socketio4j.socketio.store.redis_pubsub.RedissonStoreFactory;
114-118: Remove unusedredisConfighelper method.This method is not used anywhere in the Hazelcast test and appears to be a leftover from copy-paste.
- private Config redisConfig(String url) { - Config c = new Config(); - c.useSingleServer().setAddress(url); - return c; - }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
49-62: Consider consistent null handling for eventStoreMode.The constructor validates
hazelcastPub,hazelcastSub, andnodeIdwithrequireNonNull, but allowseventStoreModeto be null and defaults it toMULTI_CHANNEL. This inconsistency could mask configuration errors. Consider either:
- Requiring non-null and making the default explicit at the call site, or
- Documenting the default behavior clearly
public HazelcastEventStore(HazelcastInstance hazelcastPub, HazelcastInstance hazelcastSub, Long nodeId, EventStoreMode eventStoreMode) { Objects.requireNonNull(hazelcastPub, "hazelcastPub cannot be null"); Objects.requireNonNull(hazelcastSub, "hazelcastSub cannot be null"); Objects.requireNonNull(nodeId, "nodeId cannot be null"); - if (eventStoreMode == null){ - eventStoreMode = EventStoreMode.MULTI_CHANNEL; - } + // Default to MULTI_CHANNEL if not specified + this.eventStoreMode = (eventStoreMode != null) ? eventStoreMode : EventStoreMode.MULTI_CHANNEL; this.hazelcastPub = hazelcastPub; this.hazelcastSub = hazelcastSub; this.nodeId = nodeId; - this.eventStoreMode = eventStoreMode; }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (2)
41-41: Remove unused logger field.The
logfield is declared but never used in this class.public class HazelcastRingBufferStoreFactory extends BaseStoreFactory { - private static final Logger log = LoggerFactory.getLogger(HazelcastRingBufferStoreFactory.class); - private final HazelcastInstance hazelcastClient;
53-61: Add a comment or named constant to clarify the intentional null parameter.The constructor passes
nullas theringBufferNamePrefixparameter toHazelcastRingBufferEventStore, which is intentionally handled by the EventStore constructor to use theDEFAULT_RING_BUFFER_NAME_PREFIX. While this is a valid pattern for optional configuration with defaults, adding a comment here (e.g.,/* use default prefix */) or extracting a named constant would improve readability for future maintainers.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java (1)
39-39: Remove unused logger field.The
logfield is declared but never used in this class.public class RedissonStreamStoreFactory extends BaseStoreFactory { - private static final Logger log = LoggerFactory.getLogger(RedissonStreamStoreFactory.class); - private final RedissonClient redisClient;netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/AbstractEventStoreTest.java (2)
34-36: Update outdated Javadoc comment.The comment still references "PubSub store tests" but the class now tests
EventStore./** - * Abstract base class for PubSub store tests + * Abstract base class for EventStore tests */
78-134:testBasicPublishSubscribeandtestMessageFilteringare functionally identical.Both tests subscribe to
EventType.DISPATCH, publish a message from a different node, and verify receipt. The only difference is the test description comment. Either consolidate them or differentiate their test logic (e.g., one could test same-node filtering by using the same nodeId).netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/LeaveMessage.java (1)
25-37: Consider adding setters for serialization compatibility.Many serialization frameworks (e.g., Jackson for JSON, Kryo) require setters or field-level access to deserialize objects. The no-arg constructor is present, but without setters, deserialization may fail depending on the framework configuration.
If immutability is intended, mark fields
finaland ensure the serializer is configured appropriately.+ public void setSessionId(UUID sessionId) { + this.sessionId = sessionId; + } + + public void setNamespace(String namespace) { + this.namespace = namespace; + } + + public void setRoom(String room) { + this.room = room; + }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java (1)
51-52: Unused constantDEFAULT_RING_BUFFER_MAX_LENGTH.This constant is defined but never used in the implementation. Either remove it or apply it to configure the ring buffer capacity.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java (3)
102-102: Remove commented-out debug code.The commented
Thread.sleep(600)appears to be leftover debug code.port2 = cfg2.getPort(); - - //Thread.sleep(600); }
71-78: Consider extracting duplicate event listener registration.The event listeners for
join-roomandleave-roomare identical for both nodes. Consider extracting a helper method to reduce duplication.private void registerEventListeners(SocketIOServer node) { node.addEventListener("join-room", String.class, (c, room, ack) -> { c.joinRoom(room); c.sendEvent("join-ok", "OK"); }); node.addEventListener("leave-room", String.class, (c, room, ack) -> { c.leaveRoom(room); c.sendEvent("leave-ok", "OK"); }); }Also applies to: 91-98
126-128: Redundant null check on static final field.
REDIS_CONTAINERis declared asstatic finaland initialized inline, so it can never be null.- if (REDIS_CONTAINER != null) { - REDIS_CONTAINER.stop(); - } + REDIS_CONTAINER.stop();netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
120-132: Consider defensive copy before mutation.
attachOffsetmutates the packet's data list. While a new ArrayList is created from existing args, the packet itself is being modified. If the same packet instance is reused elsewhere, this could cause unintended side effects.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (1)
240-243: Null check ontopicis unnecessary.
redissonSub.getReliableTopic(...)never returns null (it creates the topic reference if it doesn't exist). The null check at lines 241-243 is dead code.RReliableTopic topic = redissonSub.getReliableTopic(getStreamName(type)); -if (topic == null) { - return; -} for (Object id : regIds) {netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (3)
51-63: Avoid reassigning ctor param; normalize mode in one expression. Also considerrequireNonNull(eventStoreMode)if you don’t want implicit defaults.- if (eventStoreMode == null){ - eventStoreMode = EventStoreMode.MULTI_CHANNEL; - } - this.eventStoreMode = eventStoreMode; + this.eventStoreMode = (eventStoreMode != null) ? eventStoreMode : EventStoreMode.MULTI_CHANNEL;
91-111: Redisson API + redundant null check + null-safety.getTopic(...)is typically non-null; if that’s true for your Redisson version,if (topic == null)is dead code. Also addObjects.requireNonNull(type)to avoid NPE.public void unsubscribe0(EventType type) { + Objects.requireNonNull(type, "type is null"); Queue<Integer> regIds = map.remove(type); if (regIds == null || regIds.isEmpty()) { return; } RTopic topic = redissonSub.getTopic(getStreamName(type)); - if (topic == null) { - return; - } for (Integer id : regIds) { try { topic.removeListener(id); } catch (Exception ex) { log.warn("Failed to remove listener {} from topic {}", id, getStreamName(type), ex); } } }
113-117: Shutdown semantics: consider blocking new subscribe/publish during shutdown.map.clear()after mass-unsubscribe is fine, but without a “closed” flag, concurrent subscribe calls during shutdown can re-add listeners and leak.
| <dependency> | ||
| <groupId>org.skyscreamer</groupId> | ||
| <artifactId>jsonassert</artifactId> | ||
| <version>1.5.3</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.jetbrains</groupId> | ||
| <artifactId>annotations</artifactId> | ||
| <version>26.0.2-1</version> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all Java source files and search for jetbrains annotations usage
rg "import org\.jetbrains\." --type java -nRepository: socketio4j/netty-socketio
Length of output: 509
🏁 Script executed:
# Also search for specific annotation usage patterns
rg "@(Nullable|NotNull|Contract|Immutable)" --type java -n | head -50Repository: socketio4j/netty-socketio
Length of output: 2987
🏁 Script executed:
# Check the overall project structure and type
git ls-files | grep -E "(pom|build|README)" | head -20Repository: socketio4j/netty-socketio
Length of output: 731
Mark org.jetbrains:annotations as provided and optional to prevent transitive leakage.
The annotations are used in public APIs (e.g., RedissonStreamEventStore constructors with @NotNull and @Nullable), so they will become transitive dependencies to consumers with default scope. Mark as provided and optional so consumers don't inherit it unintentionally.
<dependency>
<groupId>org.jetbrains</groupId>
<artifactId>annotations</artifactId>
<version>26.0.2-1</version>
+ <scope>provided</scope>
+ <optional>true</optional>
</dependency>🤖 Prompt for AI Agents
In netty-socketio-core/pom.xml around lines 161 to 171, the
org.jetbrains:annotations dependency is currently declared with default scope
which makes it transitively available to consumers; change its declaration to
mark the dependency as provided and optional so downstream projects do not
inherit it: update the dependency entry to include <scope>provided</scope> and
<optional>true</optional> (keep groupId, artifactId and version unchanged), then
run a quick mvn dependency:tree or build to verify the annotations dependency is
no longer leaked transitively.
| import com.socketio4j.socketio.store.event.DisconnectMessage; | ||
| import com.socketio4j.socketio.store.event.EventType; | ||
| import com.socketio4j.socketio.transport.PollingTransport; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd netty-socketio-core && find . -name "SocketIOChannelInitializer.java" -type fRepository: socketio4j/netty-socketio
Length of output: 140
🏁 Script executed:
# Read the file to check line count first
wc -l netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.javaRepository: socketio4j/netty-socketio
Length of output: 162
🏁 Script executed:
# Read the relevant sections
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java | sed -n '40,50p'Repository: socketio4j/netty-socketio
Length of output: 708
🏁 Script executed:
# Read the onDisconnect method and surrounding code
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java | sed -n '220,242p'Repository: socketio4j/netty-socketio
Length of output: 1018
🏁 Script executed:
# Find EventStore interface/class
find netty-socketio-core -name "EventStore.java" -o -name "*EventStore*" -type fRepository: socketio4j/netty-socketio
Length of output: 1165
🏁 Script executed:
# Read the EventStore interface
cat netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventStore.javaRepository: socketio4j/netty-socketio
Length of output: 2797
🏁 Script executed:
# Search for other publish() calls in the codebase to see if they're typically guarded
rg "\.publish\(" netty-socketio-core/src/main/java --context 2Repository: socketio4j/netty-socketio
Length of output: 8693
🏁 Script executed:
# Check the Namespace.java context to see if publish calls are wrapped
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java | grep -A 5 -B 5 "publish(EventType.BULK_LEAVE"Repository: socketio4j/netty-socketio
Length of output: 1203
🏁 Script executed:
# Let me check if there are any tests that verify exception behavior during onDisconnect
find netty-socketio-core/src/test -name "*Test.java" -type f | head -5 | xargs grep -l "onDisconnect" 2>/dev/null | head -3Repository: socketio4j/netty-socketio
Length of output: 258
🏁 Script executed:
# Verify once more that the current code truly has no exception handling
cat -n netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java | sed -n '225,234p'Repository: socketio4j/netty-socketio
Length of output: 574
Wrap eventStore().publish() in try-catch to prevent exceptions from breaking disconnect cleanup.
The EventStore.publish() method explicitly re-throws exceptions (confirmed in EventStore.java), which means an event store failure during disconnect will abort the onDisconnect() method before the debug log completes. While cleanup operations (ackManager, authorizeHandler, and storeFactory.onDisconnect) occur before the publish call, unguarded exceptions should be avoided. Recommend guarding the publish call with try-catch and logging the failure as a warning to maintain best-effort semantics for disconnect operations.
@Override
public void onDisconnect(ClientHead client) {
ackManager.onDisconnect(client);
authorizeHandler.onDisconnect(client);
configuration.getStoreFactory().onDisconnect(client);
- configuration.getStoreFactory().eventStore().publish(EventType.DISCONNECT, new DisconnectMessage(client.getSessionId()));
+ try {
+ configuration.getStoreFactory()
+ .eventStore()
+ .publish(EventType.DISCONNECT, new DisconnectMessage(client.getSessionId()));
+ } catch (Exception e) {
+ log.warn("Failed to publish DISCONNECT event for sessionId={}", client.getSessionId(), e);
+ }
log.debug("Client with sessionId: {} disconnected", client.getSessionId());
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java
around lines 45-47, the call to eventStore().publish(...) is unguarded and can
re-throw exceptions that will abort disconnect handling; wrap the publish call
in a try-catch that catches Exception, log a warning including the exception and
context (e.g., the disconnect event and session id), and continue so disconnect
cleanup and the debug log still run.
| subscribe(EventType.DISPATCH, DispatchMessage.class, | ||
| m -> { | ||
| Namespace n = hub.get(m.getNamespace()); | ||
| if (n != null) { | ||
| n.dispatch(m.getRoom(), m.getPacket()); | ||
| } | ||
| log.debug("[PUBSUB-MC] DISPATCH {}", m.getPacket()); | ||
| }); |
There was a problem hiding this comment.
Multi-channel DISPATCH handler missing offset attachment.
The single-channel handler (lines 88-97) attaches the offset to the packet before dispatching, but the multi-channel handler does not. This inconsistency could cause issues when using offsets with multi-channel mode.
Apply this diff to add offset attachment:
subscribe(EventType.DISPATCH, DispatchMessage.class,
m -> {
Namespace n = hub.get(m.getNamespace());
+ if (m.getOffset() != null && !m.getOffset().isEmpty()) {
+ attachOffset(m.getPacket(), m.getOffset());
+ }
if (n != null) {
n.dispatch(m.getRoom(), m.getPacket());
}
log.debug("[PUBSUB-MC] DISPATCH {}", m.getPacket());
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subscribe(EventType.DISPATCH, DispatchMessage.class, | |
| m -> { | |
| Namespace n = hub.get(m.getNamespace()); | |
| if (n != null) { | |
| n.dispatch(m.getRoom(), m.getPacket()); | |
| } | |
| log.debug("[PUBSUB-MC] DISPATCH {}", m.getPacket()); | |
| }); | |
| subscribe(EventType.DISPATCH, DispatchMessage.class, | |
| m -> { | |
| Namespace n = hub.get(m.getNamespace()); | |
| if (m.getOffset() != null && !m.getOffset().isEmpty()) { | |
| attachOffset(m.getPacket(), m.getOffset()); | |
| } | |
| if (n != null) { | |
| n.dispatch(m.getRoom(), m.getPacket()); | |
| } | |
| log.debug("[PUBSUB-MC] DISPATCH {}", m.getPacket()); | |
| }); |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java
around lines 162 to 169, the multi-channel DISPATCH handler logs and dispatches
the packet without attaching the message offset; modify the handler to attach
the offset to the packet the same way the single-channel handler does (retrieve
the offset from the DispatchMessage and set it on the packet before calling
Namespace.dispatch), then proceed to dispatch and log as before.
| default Long getNodeId() { | ||
| return ThreadLocalRandom.current().nextLong(Long.MAX_VALUE); | ||
| } |
There was a problem hiding this comment.
Critical: getNodeId() returns a new random value on each invocation.
This default implementation generates a new random Long every time it's called, which defeats the purpose of a node identifier. The nodeId should be consistent for the lifetime of an EventStore instance to correctly identify the source of messages.
Implementations must override this, but relying on a broken default is error-prone. Consider removing the default or throwing UnsupportedOperationException.
default Long getNodeId() {
- return ThreadLocalRandom.current().nextLong(Long.MAX_VALUE);
+ throw new UnsupportedOperationException("getNodeId() must be implemented by concrete EventStore");
}🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventStore.java
around lines 42-44, the default getNodeId() returns a new random Long on every
call which is invalid; remove the default implementation so getNodeId() becomes
an abstract method (no body) forcing concrete EventStore implementations to
provide a stable node id for their instance, or if you prefer not to break
existing code, replace the default body with "throw new
UnsupportedOperationException(...)" to avoid returning a changing value; update
method signature accordingly and run compile to find callers to implement.
| @Override | ||
| public void publish(PubSubType type, PubSubMessage msg) { | ||
| public EventStoreMode getEventStoreMode(){ | ||
| return this.eventStoreMode; | ||
| } | ||
| @Override | ||
| public void publish0(EventType type, EventMessage msg) { | ||
| msg.setNodeId(nodeId); | ||
| redissonPub.getTopic(type.toString()).publish(msg); | ||
| redissonPub.getTopic(getStreamName(type)).publish(msg); | ||
| } |
There was a problem hiding this comment.
Add null-safety for type/msg (currently NPE-prone). getStreamName(type) and msg.setNodeId(...) will blow up on null inputs.
@Override
public void publish0(EventType type, EventMessage msg) {
+ Objects.requireNonNull(type, "type is null");
+ Objects.requireNonNull(msg, "msg is null");
msg.setNodeId(nodeId);
redissonPub.getTopic(getStreamName(type)).publish(msg);
}🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java
around lines 65 to 73, the publish0 method is NPE-prone because it calls
getStreamName(type) and msg.setNodeId(nodeId) without null checks; add explicit
null-safety at the start of publish0 by validating that both type and msg are
not null (throw IllegalArgumentException or a descriptive NullPointerException)
and return/abort before using them, so you avoid downstream NPEs and provide a
clear error message when callers pass null.
| @Override | ||
| public void shutdown() { | ||
| eventStore.shutdown(); | ||
| } |
There was a problem hiding this comment.
Missing shutdown of Redisson clients - potential resource leak.
The shutdown() method only delegates to eventStore.shutdown() but does not shut down the redisClient, redisPub, or redisSub instances. Comparing to the existing RedissonStoreFactory (from relevant code snippets), which explicitly shuts down all Redisson clients in order, this could lead to resource leaks.
Consider adding client shutdown similar to the existing pattern:
@Override
public void shutdown() {
eventStore.shutdown();
+
+ // Ordered hash: preserves order, no duplicates
+ Set<RedissonClient> ordered = new LinkedHashSet<>();
+ ordered.add(redisSub);
+ ordered.add(redisPub);
+ ordered.add(redisClient);
+
+ for (RedissonClient c : ordered) {
+ if (c != null && !c.isShutdown()) {
+ try {
+ c.shutdown();
+ log.info("Shutdown: {}", c.getClass().getSimpleName());
+ } catch (Exception e) {
+ log.warn("Shutdown failed: {}", c.getClass().getSimpleName(), e);
+ }
+ }
+ }
}Note: If clients are intentionally not shut down (e.g., externally managed), document this clearly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void shutdown() { | |
| eventStore.shutdown(); | |
| } | |
| @Override | |
| public void shutdown() { | |
| eventStore.shutdown(); | |
| // Ordered hash: preserves order, no duplicates | |
| Set<RedissonClient> ordered = new LinkedHashSet<>(); | |
| ordered.add(redisSub); | |
| ordered.add(redisPub); | |
| ordered.add(redisClient); | |
| for (RedissonClient c : ordered) { | |
| if (c != null && !c.isShutdown()) { | |
| try { | |
| c.shutdown(); | |
| log.info("Shutdown: {}", c.getClass().getSimpleName()); | |
| } catch (Exception e) { | |
| log.warn("Shutdown failed: {}", c.getClass().getSimpleName(), e); | |
| } | |
| } | |
| } | |
| } |
| @Test | ||
| public void testConnectAndJoinSameRoomTest() throws Exception { | ||
| io.socket.client.IO.Options opts = new io.socket.client.IO.Options(); | ||
| opts.forceNew = true; | ||
|
|
||
| Socket a = io.socket.client.IO.socket("http://localhost:" + port1 + "?join=room1", opts); | ||
| Socket b = IO.socket("http://localhost:" + port2 + "?join=room1", opts); | ||
|
|
||
| a.connect(); | ||
| b.connect(); | ||
| CountDownLatch joinLatch = new CountDownLatch(2); | ||
| CountDownLatch connectLatch = new CountDownLatch(2); | ||
| a.on(Socket.EVENT_CONNECT, args -> { | ||
| connectLatch.countDown(); | ||
| }); | ||
| b.on(Socket.EVENT_CONNECT, args -> { | ||
| connectLatch.countDown(); | ||
| }); | ||
| assertTrue(connectLatch.await(5, TimeUnit.SECONDS), "Join timed out"); |
There was a problem hiding this comment.
Race condition: connect() called before listeners are registered.
In testConnectAndJoinSameRoomTest, sockets a and b call connect() at lines 802-803 before the EVENT_CONNECT listeners are registered at lines 806-811. This could cause the test to hang if the connection completes before listeners are attached.
Apply this diff to register listeners before connecting:
Socket a = io.socket.client.IO.socket("http://localhost:" + port1 + "?join=room1", opts);
Socket b = IO.socket("http://localhost:" + port2 + "?join=room1", opts);
-a.connect();
-b.connect();
CountDownLatch joinLatch = new CountDownLatch(2);
CountDownLatch connectLatch = new CountDownLatch(2);
a.on(Socket.EVENT_CONNECT, args -> {
connectLatch.countDown();
});
b.on(Socket.EVENT_CONNECT, args -> {
connectLatch.countDown();
});
+a.connect();
+b.connect();
assertTrue(connectLatch.await(5, TimeUnit.SECONDS), "Join timed out");🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedCommonTest.java
around lines 794 to 812, the test currently calls a.connect() and b.connect()
before attaching EVENT_CONNECT listeners which creates a race where connections
may succeed before handlers are registered; move the a.on(...) and b.on(...)
listener registrations (and any related latch setup) to before calling
a.connect() and b.connect() so listeners are in place prior to initiating the
connections, then call connect() for both sockets and await the latches as
before.
...ava/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java
Outdated
Show resolved
Hide resolved
| @BeforeAll | ||
| public void setup() throws Exception { | ||
| REDIS_CONTAINER.start(); | ||
|
|
||
| String redisURL = "redis://" + REDIS_CONTAINER.getHost() + ":" + REDIS_CONTAINER.getRedisPort(); | ||
| redisClient1 = Redisson.create(redisConfig(redisURL)); | ||
| redisClient2 = Redisson.create(redisConfig(redisURL)); | ||
| // ---------- NODE 1 ---------- | ||
| Configuration cfg1 = new Configuration(); | ||
| cfg1.setHostname("127.0.0.1"); | ||
| cfg1.setPort(findAvailablePort()); | ||
|
|
||
| cfg1.setStoreFactory(new RedissonStoreFactory( | ||
| redisClient1, PublishConfig.allUnreliable(), EventStoreMode.SINGLE_CHANNEL | ||
| )); | ||
|
|
||
| node1 = new SocketIOServer(cfg1); | ||
| node1.addEventListener("join-room", String.class, (c, room, ack) -> { | ||
| c.joinRoom(room); | ||
| c.sendEvent("join-ok", "OK"); | ||
| }); | ||
| node1.addEventListener("leave-room", String.class, (c, room, ack) -> { | ||
| c.leaveRoom(room); | ||
| c.sendEvent("leave-ok", "OK"); | ||
| }); | ||
| node1.start(); | ||
| port1 = cfg1.getPort(); | ||
|
|
||
| // ---------- NODE 2 ---------- | ||
| Configuration cfg2 = new Configuration(); | ||
| cfg2.setHostname("127.0.0.1"); | ||
| cfg2.setPort(findAvailablePort()); | ||
|
|
||
| cfg2.setStoreFactory(new RedissonStoreFactory( | ||
| redisClient2, PublishConfig.allUnreliable(), EventStoreMode.SINGLE_CHANNEL)); | ||
|
|
||
| node2 = new SocketIOServer(cfg2); | ||
| node2.addEventListener("join-room", String.class, (c, room, ack) -> { | ||
| c.joinRoom(room); | ||
| c.sendEvent("join-ok", "OK"); | ||
| }); | ||
| node2.addEventListener("leave-room", String.class, (c, room, ack) -> { | ||
| c.leaveRoom(room); | ||
| c.sendEvent("leave-ok", "OK"); | ||
| }); | ||
| node2.start(); | ||
| port2 = cfg2.getPort(); | ||
|
|
||
| //Thread.sleep(600); | ||
| } |
There was a problem hiding this comment.
Make server startup deterministic (remove “sleep” temptation, reduce CI flakes).
The commented Thread.sleep(600) suggests timing sensitivity; better to actively wait until both nodes are accepting connections / adapters subscribed.
Suggested direction (pick one):
- Poll-connect a socket.io client to
node1/node2until success (with timeout), then run assertions. - Use an await utility (e.g., Awaitility) to wait for a known-ready condition (e.g., namespace available, first ping/pong, etc.).
(Keeping this test stable matters a lot since it’s validating distributed behavior.)
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java
around lines 54 to 103, replace the commented Thread.sleep with a deterministic
wait: actively wait until both node1 and node2 are ready and accepting
connections instead of sleeping. Implement a short polling loop or use
Awaitility to attempt connecting a socket.io client (or checking a known-ready
condition such as server.isStarted(), namespace presence, or Redisson
subscription count) against each node with a sensible timeout (e.g., 5–10s) and
fail the setup if the wait times out; once both nodes respond, proceed with the
test. Ensure the wait checks are granular (retry interval ~100-200ms) and
time-bounded to avoid CI flakes.
| try (InstanceHandle<ExceptionListener> exceptionListenerInstanceHandle = Arc.container().instance(ExceptionListener.class)) { | ||
| if (exceptionListenerInstanceHandle != null && exceptionListenerInstanceHandle.isAvailable()) { | ||
| log.info("Netty socket-io server configuration uses ExceptionListener: {}", exceptionListenerInstanceHandle.get().getClass().getName()); | ||
| configuration.setExceptionListener(exceptionListenerInstanceHandle.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bean lifecycle issue: try-with-resources destroys dependent-scoped beans while configuration retains references.
The try-with-resources pattern closes the InstanceHandle after retrieving the bean, but the configuration object stores a reference to that bean and is used later (returned at line 167). In Quarkus Arc, InstanceHandle.close() destroys dependent-scoped beans. If ExceptionListener is @Dependent, the configuration will hold a reference to a destroyed bean, leading to undefined behavior or NPEs when the configuration is used.
Solution 1 (recommended): Remove try-with-resources since you're keeping the bean reference:
- try (InstanceHandle<ExceptionListener> exceptionListenerInstanceHandle = Arc.container().instance(ExceptionListener.class)) {
+ InstanceHandle<ExceptionListener> exceptionListenerInstanceHandle = Arc.container().instance(ExceptionListener.class);
+ {
if (exceptionListenerInstanceHandle != null && exceptionListenerInstanceHandle.isAvailable()) {
log.info("Netty socket-io server configuration uses ExceptionListener: {}", exceptionListenerInstanceHandle.get().getClass().getName());
configuration.setExceptionListener(exceptionListenerInstanceHandle.get());
}
}Solution 2: Document and enforce that these beans must be @ApplicationScoped or @Singleton:
// Add comment before block
// Note: Assumes ExceptionListener bean is @ApplicationScoped or @Singleton.
// @Dependent scope would cause the bean to be destroyed when the handle closes.🤖 Prompt for AI Agents
In
netty-socketio-quarkus/netty-socketio-quarkus-runtime/src/main/java/com/socketio4j/socketio/quarkus/recorder/NettySocketIOConfigRecorder.java
around lines 139-144, the try-with-resources closes the InstanceHandle which
will destroy @Dependent-scoped beans while the configuration retains a
reference; remove the try-with-resources pattern and do not close the
InstanceHandle (or otherwise avoid calling close()) when you store the bean
reference in configuration so dependent beans are not destroyed, or
alternatively document/enforce that ExceptionListener must be
@ApplicationScoped/@Singleton if you intend to close the handle.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (2)
72-79: Add null-safety fortypeandmsgparameters (NPE-prone).
getStreamName(type)andmsg.setNodeId(nodeId)will throw NPE if either parameter is null. This issue was flagged in a previous review but remains unresolved.Apply this diff:
@Override public void publish0(EventType type, EventMessage msg) { + Objects.requireNonNull(type, "type cannot be null"); + Objects.requireNonNull(msg, "msg cannot be null"); msg.setNodeId(nodeId); RTopic topic = activePubTopics.computeIfAbsent(type, k -> {
82-91: Fix race condition:computeIfAbsent(...).add(regId)can lose tracking during concurrentunsubscribe0.The two-step operation (computeIfAbsent then add) is not atomic. If
unsubscribe0removes the queue between these steps, theregIdwill be added to an orphaned queue that's no longer in the map, leaking the listener registration. This issue was flagged in a previous review but remains unresolved.Apply this diff to make the operation atomic:
public <T extends EventMessage> void subscribe0(EventType type, final EventListener<T> listener, Class<T> clazz) { + Objects.requireNonNull(type, "type cannot be null"); + Objects.requireNonNull(listener, "listener cannot be null"); + Objects.requireNonNull(clazz, "clazz cannot be null"); RTopic topic = redissonSub.getTopic(getStreamName(type)); int regId = topic.addListener(clazz, (channel, msg) -> { if (!nodeId.equals(msg.getNodeId())) { listener.onMessage(msg); } }); activeSubTopics.put(regId, topic); - map.computeIfAbsent(type, k -> new ConcurrentLinkedQueue<>()).add(regId); + map.compute(type, (k, q) -> { + if (q == null) { + q = new ConcurrentLinkedQueue<>(); + } + q.add(regId); + return q; + }); }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (1)
122-169: Constructor duplicates Builder logic.This explicit constructor duplicates the same initialization logic found in the Builder-based constructor (lines 63-110), including null checks, default value assignment, executor creation, and trim scheduling. This was already flagged in a previous review.
🧹 Nitpick comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
78-83: Consider renaming method to reflect actual purpose.The method is named
getRingBufferNamebut returns topic names for HazelcastITopic, not ring buffers. Consider renaming togetTopicNamefor clarity.-private String getRingBufferName(EventType type) { +private String getTopicName(EventType type) { if (EventStoreMode.SINGLE_CHANNEL.equals(eventStoreMode)) { return EventType.ALL_SINGLE_CHANNEL.name(); } return type.name(); }Update all call sites accordingly (lines 72, 92, 120).
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (2)
47-48: Consider consolidating redissonPub and redissonSub fields.Both fields are always assigned to the same
RedissonClientinstance in all constructors. If there's no plan to support separate publish/subscribe clients, consider using a singleredissonClientfield to reduce redundancy.
163-163: Simplify initial delay expression.The expression
Duration.ofSeconds(1).getSeconds()is unnecessarily verbose when you can use the literal1Ldirectly.Apply this diff:
- Duration.ofSeconds(1).getSeconds(), + 1L,
📜 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/store/hazelcast/HazelcastEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java
🧰 Additional context used
🧬 Code graph analysis (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessage.java (1)
EventMessage(21-44)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessage.java (1)
EventMessage(21-44)
⏰ 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). (6)
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (13)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (3)
52-65: Constructor properly validates required dependencies.The null checks for
hazelcastPub,hazelcastSub, andnodeIdare appropriate, and defaultingeventStoreModetoMULTI_CHANNELwhen null is a sensible fallback.
106-123: LGTM: Proper cleanup with error handling.The unsubscribe logic correctly handles null/empty cases and wraps listener removal in try-catch to prevent partial failures from blocking cleanup.
126-132: LGTM: Shutdown properly cleans up without terminating clients.The shutdown sequence unsubscribes all event types, clears internal state, and correctly avoids shutting down the Hazelcast clients (which may be shared).
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (4)
53-65: Constructor properly validates required dependencies.The null checks for
redissonPub,redissonSub, andnodeIdare appropriate, and defaultingeventStoreModetoMULTI_CHANNELwhen null is a sensible fallback.
92-97: LGTM: Stream name resolution correctly handles single-channel mode.The method properly returns the unified channel name for SINGLE_CHANNEL mode or the event-type-specific name otherwise.
99-116: LGTM: Proper cleanup with error handling.The unsubscribe logic correctly handles null/empty cases and wraps listener removal in try-catch to prevent partial failures from blocking cleanup.
119-124: LGTM: Shutdown properly cleans up without terminating clients.The shutdown sequence unsubscribes all event types and clears internal state without shutting down the Redisson clients (which may be shared).
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (6)
170-183: LGTM!The mode-specific trimming logic correctly handles SINGLE_CHANNEL vs. MULTI_CHANNEL modes, and exception handling is appropriate.
185-198: LGTM!The use of
trimNonStrictAsyncwithnoLimit()implements efficient approximate MAXLEN trimming. Exception handling and logging are appropriate.
210-217: LGTM!The message origin tracking via
nodeIdassignment and thread-safe lazy topic creation usingcomputeIfAbsentare correctly implemented.
220-232: LGTM!The nodeId filtering prevents message echo-back, and dual registration tracking (in
activeSubTopicsandmap) properly supports cleanup during unsubscription.
242-261: LGTM!The unsubscription logic correctly removes registrations, removes listeners, and handles exceptions gracefully.
272-311: LGTM!The Builder pattern is correctly implemented with required and optional fields, following standard fluent API conventions.
| public void publish0(EventType type, EventMessage msg) { | ||
| msg.setNodeId(nodeId); | ||
|
|
||
| ITopic<EventMessage> topic = activePubTopics.computeIfAbsent(type, k -> { | ||
| String topicName = getRingBufferName(k); | ||
| return hazelcastPub.getTopic(topicName); | ||
| }); | ||
|
|
||
| topic.publish(msg); | ||
| } |
There was a problem hiding this comment.
Add null-safety for type and msg parameters (NPE-prone).
getRingBufferName(type) and msg.setNodeId(nodeId) will throw NPE if either parameter is null. Add explicit validation at the method entry.
Apply this diff:
@Override
public void publish0(EventType type, EventMessage msg) {
+ Objects.requireNonNull(type, "type cannot be null");
+ Objects.requireNonNull(msg, "msg cannot be null");
msg.setNodeId(nodeId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void publish0(EventType type, EventMessage msg) { | |
| msg.setNodeId(nodeId); | |
| ITopic<EventMessage> topic = activePubTopics.computeIfAbsent(type, k -> { | |
| String topicName = getRingBufferName(k); | |
| return hazelcastPub.getTopic(topicName); | |
| }); | |
| topic.publish(msg); | |
| } | |
| public void publish0(EventType type, EventMessage msg) { | |
| Objects.requireNonNull(type, "type cannot be null"); | |
| Objects.requireNonNull(msg, "msg cannot be null"); | |
| msg.setNodeId(nodeId); | |
| ITopic<EventMessage> topic = activePubTopics.computeIfAbsent(type, k -> { | |
| String topicName = getRingBufferName(k); | |
| return hazelcastPub.getTopic(topicName); | |
| }); | |
| topic.publish(msg); | |
| } |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java
around lines 68–77, add explicit null checks for the method parameters so
getRingBufferName(type) and msg.setNodeId(nodeId) cannot NPE: validate that both
'type' and 'msg' are non-null at the start of publish0 (use
Objects.requireNonNull(...) or explicit if-checks) and throw a clear
NullPointerException/IllegalArgumentException with a message if either is null;
only after these checks proceed to computeIfAbsent/getTopic and call
msg.setNodeId(nodeId) and topic.publish(msg).
| public <T extends EventMessage> void subscribe0(EventType type, final EventListener<T> listener, Class<T> clazz) { | ||
|
|
||
| ITopic<T> topic = hazelcastSub.getTopic(getRingBufferName(type)); | ||
|
|
||
| UUID regId = topic.addMessageListener(msg -> { | ||
| if (!nodeId.equals(msg.getMessageObject().getNodeId())) { | ||
| listener.onMessage(msg.getMessageObject()); | ||
| } | ||
| }); | ||
| activeSubTopics.put(regId, topic); | ||
|
|
||
| listenerMap.computeIfAbsent(type, k -> new ConcurrentLinkedQueue<>()) | ||
| .add(regId); | ||
| } |
There was a problem hiding this comment.
Fix race condition: computeIfAbsent(...).add(regId) can lose tracking during concurrent unsubscribe0.
The two-step operation (computeIfAbsent then add) is not atomic. If unsubscribe0 removes the queue between these steps, the regId will be added to an orphaned queue that's no longer in the map, leaking the listener registration.
Apply this diff to make the operation atomic:
public <T extends EventMessage> void subscribe0(EventType type, final EventListener<T> listener, Class<T> clazz) {
+ Objects.requireNonNull(type, "type cannot be null");
+ Objects.requireNonNull(listener, "listener cannot be null");
+ Objects.requireNonNull(clazz, "clazz cannot be null");
ITopic<T> topic = hazelcastSub.getTopic(getRingBufferName(type));
UUID regId = topic.addMessageListener(msg -> {
if (!nodeId.equals(msg.getMessageObject().getNodeId())) {
listener.onMessage(msg.getMessageObject());
}
});
activeSubTopics.put(regId, topic);
- listenerMap.computeIfAbsent(type, k -> new ConcurrentLinkedQueue<>())
- .add(regId);
+ listenerMap.compute(type, (k, q) -> {
+ if (q == null) {
+ q = new ConcurrentLinkedQueue<>();
+ }
+ q.add(regId);
+ return q;
+ });
}🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java
around lines 90 to 103, the current two-step computeIfAbsent(...).add(regId) is
racy because unsubscribe0 may remove the queue between creation and add; change
this to an atomic map.compute(...) (or equivalent synchronized block) that
creates the ConcurrentLinkedQueue if null, adds regId into that queue, and
returns the queue in a single atomic operation so the registration ID cannot be
appended to a queue that was concurrently removed.
| if (b.streamNamePrefix == null || b.streamNamePrefix.isEmpty()) { | ||
| this.streamNamePrefix = | ||
| DEFAULT_STREAM_NAME_PREFIX; | ||
| } else { | ||
| this.streamNamePrefix = | ||
| b.streamNamePrefix; | ||
| } |
There was a problem hiding this comment.
Inconsistent default value logging behavior between constructors.
The explicit constructor (lines 131-141) logs warnings when applying default values for streamNamePrefix and streamMaxLength, but the Builder constructor (lines 78-92) silently applies the same defaults without logging. This inconsistency can confuse users about when defaults are applied.
Consider either:
- Adding the same warning logs to the Builder constructor path, or
- Removing the warnings from the explicit constructor if silent defaults are acceptable.
Also applies to: 86-92, 131-141
🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java
around lines 78-92 and 131-141, the Builder-based constructor silently applies
defaults while the explicit constructor logs warnings; make behavior consistent
by adding the same warning logs in the Builder path when defaulting
streamNamePrefix and streamMaxLength (or alternatively remove the warnings from
the explicit ctor if you prefer silent defaults) — implement the chosen approach
by using the same logger and message text/format used in the explicit
constructor at the Builder assignments for streamNamePrefix and streamMaxLength,
and ensure you do not produce duplicate messages when both constructors run.
...-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java (1)
69-72: Missing shutdown of RedissonClient - potential resource leak.The
shutdown()method only delegates toeventStore.shutdown()but does not shut down theredissonClient. If this factory owns the client lifecycle, this will leak Redis connections.If the
redissonClientis externally managed, document this clearly. Otherwise, consider shutting it down:@Override public void shutdown() { eventStore.shutdown(); + if (redissonClient != null && !redissonClient.isShutdown()) { + try { + redissonClient.shutdown(); + } catch (Exception e) { + log.warn("Failed to shutdown RedissonClient", e); + } + } }netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (1)
103-105: Consider deterministic readiness check instead of sleep.The commented
Thread.sleep(600)suggests timing sensitivity. For CI stability, consider using a deterministic wait (e.g., Awaitility or polling for server readiness) rather than arbitrary sleeps.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
24-38: Remove unused Redisson imports - copy-paste artifacts.This is a Hazelcast test but includes unused Redis-related imports. This was flagged in a previous review.
import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.TestInstance; -import org.redisson.Redisson; -import org.redisson.api.RedissonClient; -import org.redisson.config.Config; import com.hazelcast.client.HazelcastClient; import com.hazelcast.client.config.ClientConfig; import com.hazelcast.core.HazelcastInstance; import com.socketio4j.socketio.Configuration; import com.socketio4j.socketio.SocketIOServer; import com.socketio4j.socketio.store.CustomizedHazelcastContainer; -import com.socketio4j.socketio.store.CustomizedRedisContainer; import com.socketio4j.socketio.store.event.EventStoreMode; import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferEventStore; import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferStoreFactory; -import com.socketio4j.socketio.store.redis_stream.RedissonStreamStoreFactory;netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (1)
243-251: Redundantshutdown()call beforeshutdownNow().Line 244 calls
trimExecutor.shutdown()and line 247 immediately callstrimExecutor.shutdownNow(). Theshutdown()call is redundant becauseshutdownNow()already handles stopping the executor.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (3)
85-95: Add null-safety fortypeandmsgparameters.
getTopicName(type)andmsg.setNodeId(nodeId)will throw NPE if either parameter is null. Add explicit validation at the method entry.
119-121: Race condition:computeIfAbsent(...).add(regId)can lose tracking during concurrentunsubscribe0.The two-step operation is not atomic. If
unsubscribe0removes the queue between these steps, theregIdwill be added to an orphaned queue, leaking the listener registration.
191-197: Incorrect error message references wrong field name.The error message says "ringBufferNamePrefix" but this method sets
topicNamePrefix. This appears to be a copy-paste error fromHazelcastRingBufferEventStore.Apply this diff:
public HazelcastEventStore.Builder topicNamePrefix(@NotNull String prefix) { if (prefix.isEmpty()) { - throw new IllegalArgumentException("ringBufferNamePrefix cannot be empty"); + throw new IllegalArgumentException("topicNamePrefix cannot be empty"); } this.topicNamePrefix = prefix; return this; }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (2)
89-97: Add null-safety fortypeandmsgparameters.
getStreamName(type)andmsg.setNodeId(nodeId)will throw NPE if either parameter is null. Add explicit validation at the method entry.
99-109: Race condition:computeIfAbsent(...).add(regId)can lose tracking during concurrentunsubscribe0.The two-step operation is not atomic. If
unsubscribe0removes the queue between these steps, theregIdwill be added to an orphaned queue, leaking the listener registration.
🧹 Nitpick comments (22)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java (1)
19-34: Remove unused imports.Several imports are unused in this file:
Duration(line 19)Redisson(line 24)EventStoreMode(line 32)-import java.time.Duration; import java.util.Map; import java.util.Objects; import java.util.UUID; -import org.redisson.Redisson; import org.redisson.api.RedissonClient; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.socketio4j.socketio.store.Store; import com.socketio4j.socketio.store.event.BaseStoreFactory; import com.socketio4j.socketio.store.event.EventStore; -import com.socketio4j.socketio.store.event.EventStoreMode; import com.socketio4j.socketio.store.redis_pubsub.RedissonStore;netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
32-34: Remove unused import.
RedissonEventStoreis imported but not used in this test (it usesRedissonStreamEventStoreinstead).import com.socketio4j.socketio.store.event.EventStoreMode; -import com.socketio4j.socketio.store.redis_pubsub.RedissonEventStore; import com.socketio4j.socketio.store.redis_stream.RedissonStreamEventStore; import com.socketio4j.socketio.store.redis_stream.RedissonStreamStoreFactory;netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
56-58: Fix misleading comment - this is Hazelcast, not Redis.The comment says "Redis + Node Setup" but this test uses Hazelcast.
// ------------------------------------------- - // Redis + Node Setup + // Hazelcast + Node Setup // -------------------------------------------netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java (3)
40-50: Potential flaky port allocation (TOCTOU) infindAvailablePort().
ServerSocket(0)+ close can lose the port beforeSocketIOServerbinds, especially under parallel CI. Consider using a port utility (if available) or letting the server bind0directly (if supported) and reading the actual bound port afterwards. Also, the “unencrypted socket” static-analysis hint here is a false positive: this socket is only for port discovery, not data transport.
55-105: Test “unreliable” semantics aren’t apparent from configuration.
Class name saysUnReliable, but the event store is only configured withEventStoreMode.MULTI_CHANNEL. If “unreliable” is meant to disable retries/acks, please wire that explicitly (or rename the test to match actual behavior).
113-131: Make teardown exception-safe to avoid resource leaks on partial failures.
Ifnode1.stop()(or any shutdown) throws, the remaining resources won’t be cleaned up, which can cascade into later test failures. Consider guarding each close withtry/catch(or a small helper) to ensure all resources attempt shutdown.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (1)
51-79: LGTM: clean factory wrapper around Hazelcast-backedStore+EventStore.
One small API footgun: sinceHazelcastRingBufferEventStoreis passed in, consider documenting (or asserting) that theeventStorewas built against the sameHazelcastInstanceashazelcastClient, otherwise you can end up storing session data in one cluster and publishing events on another.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (3)
46-53: Potential flaky port allocation (TOCTOU) infindAvailablePort().
Same issue as other distributed tests: the chosen port can be taken before bind. IfConfiguration#setPort(0)works, that’s usually the least flaky option.
55-120: Remove leftover Redis-specific helper/comment to reduce confusion.
This is a Hazelcast single-channel test, but it still contains “Redis + Node Setup” and a Redis config helper. Suggest trimming that to keep the scenario clear.- // ------------------------------------------- - // Redis + Node Setup - // ------------------------------------------- + // ------------------------------------------- + // Hazelcast + Node Setup + // ------------------------------------------- @@ - private Config redisConfig(String url) { - Config c = new Config(); - c.useSingleServer().setAddress(url); - return c; - } + // (no Redis helpers in this test)
121-139: Make teardown exception-safe to avoid leaks impacting later tests.
Wrap each stop/shutdown intry/catchso one failure doesn’t prevent cleanup of the rest.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java (3)
45-49: Potential flaky port allocation (TOCTOU) infindAvailablePort().
Consider letting the server bind to port0(if supported) or using a stable port allocator utility to reduce CI flakes.
111-129: Make teardown exception-safe to avoid cross-test interference.
Guard each stop/shutdown so all resources attempt cleanup even if one throws.
36-38: Consider normalizingUnReliablevsUnreliablenaming across tests.
This is minor, but it helps discoverability and consistency.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
49-77: Document/guardredisClient↔eventStorepairing to prevent misconfiguration.
Since callers can pass anyEventStore, it’s easy to accidentally use a store factory whose session data goes to one Redis while events are published on another. Consider documenting that they must be created from the sameRedissonClient(or add a defensive check if the event store exposes its client). Also: it’s good thatshutdown()doesn’t implicitly shutdownredisClient—callers clearly own the client lifecycle.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (2)
31-32: Remove unused imports.
EventStoreModeandPublishConfigare imported but not used in this class.Apply this diff:
-import com.socketio4j.socketio.store.event.EventStoreMode; -import com.socketio4j.socketio.store.event.PublishConfig;
41-41: Remove unused logger field.The
logfield is declared but never used in this class.Apply this diff:
- private static final Logger log = LoggerFactory.getLogger(HazelcastStoreFactory.class); - private final HazelcastInstance hazelcastClient;If logging is intended for future use, consider adding it when needed.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (2)
24-26: Remove unused imports.The Redisson-related imports (
Redisson,RedissonClient,Config,CustomizedRedisContainer,RedissonStreamStoreFactory) andHazelcastEventStoreare not used in this test class.Apply this diff:
import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.TestInstance; -import org.redisson.Redisson; -import org.redisson.api.RedissonClient; -import org.redisson.config.Config; import com.hazelcast.client.HazelcastClient; import com.hazelcast.client.config.ClientConfig; import com.hazelcast.core.HazelcastInstance; import com.socketio4j.socketio.Configuration; import com.socketio4j.socketio.SocketIOServer; import com.socketio4j.socketio.store.CustomizedHazelcastContainer; -import com.socketio4j.socketio.store.CustomizedRedisContainer; import com.socketio4j.socketio.store.event.EventStoreMode; -import com.socketio4j.socketio.store.hazelcast.HazelcastEventStore; import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferEventStore; import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferStoreFactory; -import com.socketio4j.socketio.store.redis_stream.RedissonStreamStoreFactory;Also applies to: 34-39
57-59: Fix misleading comment.The comment says "Redis + Node Setup" but this test sets up Hazelcast nodes.
Apply this diff:
- // ------------------------------------------- - // Redis + Node Setup - // ------------------------------------------- + // ------------------------------------------- + // Hazelcast + Node Setup + // -------------------------------------------netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
28-29: Update outdated Javadoc.The Javadoc still references "HazelcastPubSubStore" which no longer exists.
Apply this diff:
/** - * Test class for HazelcastPubSubStore using testcontainers + * Test class for HazelcastRingBufferEventStore using testcontainers */netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStreamStoreFactoryTest.java (1)
106-111: Update assertion messages to match EventStore terminology.The assertion messages still reference the old
PubSubStoreterminology while the code now usesEventStore. This inconsistency can cause confusion during test failures.Apply this diff:
public void testRedissonEventStore() { EventStore eventStore = storeFactory.eventStore(); - assertNotNull(eventStore, "PubSubStore should not be null"); - assertTrue(eventStore instanceof RedissonEventStore, "PubSubStore should be RedissonPubSubStore"); + assertNotNull(eventStore, "EventStore should not be null"); + assertTrue(eventStore instanceof RedissonEventStore, "EventStore should be RedissonEventStore"); }netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (1)
109-123: Executor created unconditionally even when trimming is disabled.The
ScheduledExecutorServiceis always instantiated (lines 109-114), but only used whentrimEvery != null. This wastes a thread when trimming is not configured.Consider lazy initialization:
- ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(r -> { - Thread t = new Thread(r, "socketio4j-redis-stream-trimmer"); - t.setDaemon(true); - return t; - }); - this.trimExecutor = executorService; this.trimEvery = trimEvery; if (trimEvery != null) { + ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "socketio4j-redis-stream-trimmer"); + t.setDaemon(true); + return t; + }); + this.trimExecutor = executorService; executorService.scheduleAtFixedRate( this::trimAllReliableStreams, Duration.ofSeconds(1).getSeconds(), trimEvery.getSeconds(), TimeUnit.SECONDS ); + } else { + this.trimExecutor = null; }Then guard shutdown accordingly:
public void shutdown0() { - trimExecutor.shutdown(); + if (trimExecutor != null) { + trimExecutor.shutdownNow(); + } Arrays.stream(EventType.values()).forEach(this::unsubscribe); map.clear(); - trimExecutor.shutdownNow();netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (1)
110-115: Missing topic name prefix support for namespace isolation.Unlike
RedissonStreamEventStore(which hasstreamNamePrefix) andHazelcastEventStore(which hastopicPrefix), this implementation uses rawEventType.name()without any configurable prefix. This can cause topic collisions when multiple applications share the same Redis instance.Consider adding a
topicNamePrefixfield and Builder method for consistency with sibling implementations:+ private static final String DEFAULT_TOPIC_NAME_PREFIX = "SOCKETIO4J:"; + private final String topicNamePrefix; // In constructor: + this.topicNamePrefix = topicNamePrefix != null ? topicNamePrefix : DEFAULT_TOPIC_NAME_PREFIX; private String getStreamName(EventType type) { if (EventStoreMode.SINGLE_CHANNEL.equals(eventStoreMode)) { - return EventType.ALL_SINGLE_CHANNEL.name(); + return topicNamePrefix + EventType.ALL_SINGLE_CHANNEL.name(); } - return type.name(); + return topicNamePrefix + type.name(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastRingBufferStoreFactoryTest.java(3 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStreamStoreFactoryTest.java(4 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonStreamEventStoreTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java
🧰 Additional context used
🧬 Code graph analysis (12)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStore.java (1)
RedissonStore(27-60)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (1)
RedissonStreamEventStore(47-336)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java (1)
RedissonStreamStoreFactory(36-80)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (3)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java (1)
CustomizedHazelcastContainer(35-84)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
HazelcastEventStore(41-214)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
HazelcastStoreFactory(39-81)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStore.java (1)
HazelcastStore(26-59)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (1)
RedissonStreamEventStore(47-336)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java (1)
RedissonStreamStoreFactory(36-80)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonStreamStoreFactoryTest.java (4)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/PublishConfig.java (1)
PublishConfig(28-71)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (1)
RedissonEventStore(43-200)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStore.java (1)
RedissonStore(27-60)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
RedissonStoreFactory(37-79)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessage.java (1)
EventMessage(21-44)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (1)
Builder(144-198)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessage.java (1)
EventMessage(21-44)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java (1)
Builder(162-222)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/PublishConfig.java (1)
PublishConfig(28-71)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastRingBufferStoreFactoryTest.java (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
HazelcastEventStore(41-214)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java (1)
HazelcastRingBufferEventStore(42-225)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (1)
HazelcastRingBufferStoreFactory(39-81)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java (1)
CustomizedHazelcastContainer(35-84)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
HazelcastEventStore(41-214)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/Configuration.java (1)
Configuration(32-193)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java (1)
CustomizedHazelcastContainer(35-84)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
HazelcastEventStore(41-214)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java (1)
HazelcastRingBufferEventStore(42-225)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (1)
HazelcastRingBufferStoreFactory(39-81)
🪛 ast-grep (0.40.0)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java
[info] 50-50: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubMultiChannelUnReliableTest.java
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java
[info] 49-49: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubMultiChannelUnReliableTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java
[info] 51-51: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-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). (6)
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (9)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonPubSubSingleChannelUnreliableTest.java (1)
113-131: Teardown logic is well-structured.Proper null checks and ordered shutdown of nodes → clients → container. This follows the correct inverse order of initialization.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
55-107: Setup structure looks correct for multi-channel stream test.Both nodes are correctly configured with
RedissonStreamStoreFactoryandEventStoreMode.MULTI_CHANNEL. The event handlers for join-room/leave-room are appropriately registered.Consider removing or replacing the commented
Thread.sleep(600)with a deterministic readiness check for CI stability.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
54-104: Setup correctly configures SINGLE_CHANNEL mode for stream-based event store.The test properly wires
RedissonStreamStoreFactorywithEventStoreMode.SINGLE_CHANNEL, matching the intended test scenario. Both nodes share the same Redis backend via separate clients.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
59-115: Hazelcast ring buffer setup is correctly structured.Both nodes are properly configured with
HazelcastRingBufferStoreFactoryandEventStoreMode.SINGLE_CHANNEL. The Hazelcast client configuration withsetSmartRouting(false)is appropriate for containerized testing.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
51-58: Constructor and factory implementation look good.The constructor properly validates non-null arguments and stores the dependencies. The factory methods correctly delegate to the appropriate components, following the same pattern as
HazelcastRingBufferStoreFactory.netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastRingBufferStoreFactoryTest.java (1)
59-76: Test setup correctly wires HazelcastRingBufferStoreFactory.The factory is properly constructed with a
HazelcastRingBufferEventStorebuilt via its Builder pattern, consistent with the new EventStore architecture.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (2)
51-55: Safe usage for ephemeral port discovery.The static analysis warning about unencrypted socket is a false positive here. Using
ServerSocket(0)is a standard, safe technique for finding an available ephemeral port in tests—no sensitive data is transmitted. The socket is immediately closed after retrieving the port.
60-116: Distributed test setup looks correct.The test properly initializes two independent Hazelcast-backed Socket.IO nodes with
HazelcastRingBufferStoreFactoryinMULTI_CHANNELmode, allowing verification of cross-node communication via the event store.netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
36-55: Test setup and teardown structure is appropriate.The test correctly manages the Hazelcast container lifecycle with reuse enabled and properly creates separate pub/sub client instances.
| private String getTopicName(EventType type) { | ||
| if (EventStoreMode.SINGLE_CHANNEL.equals(eventStoreMode)) { | ||
| return EventType.ALL_SINGLE_CHANNEL.name(); | ||
| } | ||
| return type.name(); | ||
| } |
There was a problem hiding this comment.
topicPrefix not applied in SINGLE_CHANNEL mode.
In getTopicName(), the topicPrefix is not prepended when SINGLE_CHANNEL mode is used, unlike the MULTI_CHANNEL path. This creates inconsistent topic naming and may cause topic collisions across different applications.
Apply this diff:
private String getTopicName(EventType type) {
if (EventStoreMode.SINGLE_CHANNEL.equals(eventStoreMode)) {
- return EventType.ALL_SINGLE_CHANNEL.name();
+ return topicPrefix + EventType.ALL_SINGLE_CHANNEL.name();
}
- return type.name();
+ return topicPrefix + type.name();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private String getTopicName(EventType type) { | |
| if (EventStoreMode.SINGLE_CHANNEL.equals(eventStoreMode)) { | |
| return EventType.ALL_SINGLE_CHANNEL.name(); | |
| } | |
| return type.name(); | |
| } | |
| private String getTopicName(EventType type) { | |
| if (EventStoreMode.SINGLE_CHANNEL.equals(eventStoreMode)) { | |
| return topicPrefix + EventType.ALL_SINGLE_CHANNEL.name(); | |
| } | |
| return topicPrefix + type.name(); | |
| } |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java
around lines 96 to 101, the SINGLE_CHANNEL branch of getTopicName() returns
EventType.ALL_SINGLE_CHANNEL.name() without applying topicPrefix; update the
method so that when eventStoreMode is SINGLE_CHANNEL it returns topicPrefix +
EventType.ALL_SINGLE_CHANNEL.name() (matching the MULTI_CHANNEL path), taking
care to handle a null/empty topicPrefix consistently (e.g., treat null as empty
string) so topic naming remains consistent and avoids collisions.
| import org.jetbrains.annotations.NotNull; | ||
| import org.redisson.Redisson; | ||
| import org.redisson.api.RedissonClient; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import com.hazelcast.internal.services.ObjectNamespace; | ||
| import com.socketio4j.socketio.store.Store; | ||
| import com.socketio4j.socketio.store.event.BaseStoreFactory; | ||
| import com.socketio4j.socketio.store.event.EventStore; | ||
| import com.socketio4j.socketio.store.event.EventStoreMode; | ||
| import com.socketio4j.socketio.store.event.PublishConfig; | ||
|
|
There was a problem hiding this comment.
Avoid depending on Hazelcast “internal” packages from the Redis module.
com.hazelcast.internal.services.ObjectNamespace is an internal API and shouldn’t be referenced here (even as an import), since it can break compilation across Hazelcast versions and signals unintended coupling.
-import com.hazelcast.internal.services.ObjectNamespace;🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
around lines 23 to 35 the file imports the internal Hazelcast API
com.hazelcast.internal.services.ObjectNamespace which is an internal, unstable
dependency; remove that import and any usages of ObjectNamespace in this Redis
module and replace them with a project-owned type or a plain String/UUID-based
namespace representation (or use a public Hazelcast API if you truly need
Hazelcast types). Ensure all references are updated to the new type or string
constant, update any constructors/method signatures that referenced
ObjectNamespace, and run compilation/tests to verify no remaining references to
com.hazelcast.internal.* remain.
| if (streamMaxLength != Integer.MAX_VALUE) { | ||
| trimEvery = Duration.ofSeconds(60); | ||
| } |
There was a problem hiding this comment.
User-provided trimEvery is silently overwritten.
When streamMaxLength != Integer.MAX_VALUE, the user-provided trimEvery parameter is unconditionally overwritten with a 60-second default. This ignores the caller's explicit configuration and could lead to unexpected behavior.
Apply this diff to preserve user-provided values:
- if (streamMaxLength != Integer.MAX_VALUE) {
- trimEvery = Duration.ofSeconds(60);
- }
+ if (streamMaxLength != Integer.MAX_VALUE && trimEvery == null) {
+ trimEvery = Duration.ofSeconds(60);
+ log.info("trimEvery defaulted to 60s because streamMaxLength is set");
+ }🤖 Prompt for AI Agents
In
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamEventStore.java
around lines 98 to 100, the code unconditionally overwrites the user-provided
trimEvery when streamMaxLength != Integer.MAX_VALUE; change the logic to only
set the 60-second default when trimEvery is null (or not provided) so callers'
explicit values are preserved — i.e., check that trimEvery == null before
assigning Duration.ofSeconds(60); keep the existing behavior when trimEvery is
already set.
| import com.socketio4j.socketio.store.hazelcast.HazelcastEventStore; | ||
|
|
||
| /** | ||
| * Test class for HazelcastPubSubStore using testcontainers | ||
| */ | ||
| public class HazelcastPubSubStoreTest extends AbstractPubSubStoreTest { | ||
| public class HazelcastRingBufferEventStoreTest extends AbstractEventStoreTest { |
There was a problem hiding this comment.
Class name and implementation mismatch: uses HazelcastEventStore instead of HazelcastRingBufferEventStore.
The test class is named HazelcastRingBufferEventStoreTest but creates a HazelcastEventStore via HazelcastEventStore.Builder. Based on the class name and the relevant code snippets showing HazelcastRingBufferEventStore.Builder exists, this should use the ring buffer variant.
Apply this diff:
-import com.socketio4j.socketio.store.hazelcast.HazelcastEventStore;
+import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferEventStore;- return new HazelcastEventStore.Builder(hazelcastPub, hazelcastSub).build();
+ return new HazelcastRingBufferEventStore.Builder(hazelcastPub, hazelcastSub).build();Also applies to: 54-54
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java
around lines 26-31 (and also at line 54), the test constructs a
HazelcastEventStore but the class name indicates it should test the ring-buffer
variant; change the import to use
com.socketio4j.socketio.store.hazelcast.HazelcastRingBufferEventStore, update
any instantiation/Builder calls to HazelcastRingBufferEventStore.Builder
(instead of HazelcastEventStore.Builder), and adjust any variable types and
imports accordingly so the test creates and asserts against the ring-buffer
implementation.
| import com.socketio4j.socketio.store.redis_pubsub.RedissonEventStore; | ||
|
|
||
| /** | ||
| * Test class for RedissonPubSubStore using testcontainers | ||
| * Test class for RedissonStreamEventStore using testcontainers | ||
| */ | ||
| public class RedissonPubSubStoreTest extends AbstractPubSubStoreTest { | ||
| public class RedissonStreamEventStoreTest extends AbstractEventStoreTest { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for RedissonStreamEventStore class definition ==="
rg -t java "class.*RedissonStreamEventStore" -A 2
echo ""
echo "=== Searching for redis_stream directory ==="
fd -t d "redis_stream"
echo ""
echo "=== Listing redis package directories ==="
fd -t d "redis" | grep -E "redis_|redis/" | head -20
echo ""
echo "=== Checking EventStore implementations in redis-related packages ==="
rg -t java "implements.*EventStore" | grep -i redis
echo ""
echo "=== Examining the test file content ==="
cat -n netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonStreamEventStoreTest.javaRepository: socketio4j/netty-socketio
Length of output: 4765
Import and implementation mismatch: test imports pub/sub store instead of stream store.
The test class is named RedissonStreamEventStoreTest with documentation stating "Test class for RedissonStreamEventStore", but it imports and instantiates RedissonEventStore from the redis_pubsub package (line 25 and 49). The correct stream-based implementation RedissonStreamEventStore exists in the redis_stream package and should be used instead to match the class name and intent.
| public void testHazelcastEventStore() { | ||
| EventStore eventStore = storeFactory.eventStore(); | ||
|
|
||
| assertNotNull(pubSubStore, "PubSubStore should not be null"); | ||
| assertInstanceOf(HazelcastPubSubStore.class, pubSubStore, "PubSubStore should be HazelcastStore"); | ||
| assertNotNull(eventStore, "EventStore should not be null"); | ||
| assertInstanceOf(HazelcastEventStore.class, eventStore, "EventStore should be HazelcastStore"); | ||
| } |
There was a problem hiding this comment.
Incorrect assertion type: should check for HazelcastRingBufferEventStore.
The test creates a HazelcastRingBufferStoreFactory with a HazelcastRingBufferEventStore, but the assertion checks for HazelcastEventStore.class. These are different classes, so this assertion will fail.
Apply this diff:
@Test
public void testHazelcastEventStore() {
EventStore eventStore = storeFactory.eventStore();
assertNotNull(eventStore, "EventStore should not be null");
- assertInstanceOf(HazelcastEventStore.class, eventStore, "EventStore should be HazelcastStore");
+ assertInstanceOf(HazelcastRingBufferEventStore.class, eventStore, "EventStore should be HazelcastRingBufferEventStore");
}🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastRingBufferStoreFactoryTest.java
around lines 114 to 119, the test asserts the wrong concrete class
(HazelcastEventStore) while the factory produces a
HazelcastRingBufferEventStore; update the assertion to
assertInstanceOf(HazelcastRingBufferEventStore.class, eventStore, "EventStore
should be HazelcastRingBufferEventStore") so the test checks for the correct
implementation and adjust the failure message accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (3)
39-39: Unused logger field.The logger is declared but never used in this class. Consider removing it or adding relevant log statements (e.g., in the constructor or shutdown method for lifecycle tracking).
44-47: Clarify the API version comment.The comment "API 4.y.z" is vague and doesn't provide meaningful information about the constructor's purpose or the API contract it represents.
Consider replacing it with a more descriptive comment:
- /** - * API 4.y.z - * @param hazelcastClient - * @param eventStore - */ + /** + * Creates a new Hazelcast ring-buffer store factory. + * + * @param hazelcastClient the Hazelcast client instance for creating stores + * @param eventStore the ring-buffer-backed event store for pub/sub operations + */
49-77: Implementation looks solid, but note the code duplication.The class correctly implements the BaseStoreFactory contract with proper null checks and delegation. However, this implementation is nearly identical to
HazelcastStoreFactory.java(lines 48-76), differing only in the constructor parameter type (HazelcastRingBufferEventStorevsHazelcastEventStore). Consider extracting common logic into a shared base class or abstract factory method if more Hazelcast-based factories are planned.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (2)
38-38: Unused logger field.The logger is declared but never used in this class. Consider removing it or adding relevant log statements (e.g., in the constructor or shutdown method for lifecycle tracking).
43-47: Clarify the API version comment.The comment "API 4.y.z" is vague and doesn't provide meaningful information about the constructor's purpose or the API contract it represents.
Consider replacing it with a more descriptive comment:
- /** - * API 4.y.z - * @param hazelcastClient - * @param eventStore - */ + /** + * Creates a new Hazelcast store factory. + * + * @param hazelcastClient the Hazelcast client instance for creating stores + * @param eventStore the Hazelcast-backed event store for pub/sub operations + */
📜 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/store/hazelcast/HazelcastStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedissonStreamStoreFactory.java
- netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java
🧰 Additional context used
🧬 Code graph analysis (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferStoreFactory.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStore.java (1)
HazelcastStore(26-59)
⏰ 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 (17) / build
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
🔇 Additional comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
48-76: Implementation looks solid.The class correctly implements the BaseStoreFactory contract with proper null checks and delegation. The code duplication with
HazelcastRingBufferStoreFactory.javahas been noted in that file's review.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (1)
26-26: Class name and implementation mismatch: usesHazelcastEventStoreinstead ofHazelcastRingBufferEventStore.The test class is named
HazelcastRingBufferEventStoreTestbut creates aHazelcastEventStoreviaHazelcastEventStore.Builder. Based on the class name and the relevant code snippets showingHazelcastRingBufferEventStoreis used in other tests (e.g.,DistributedHazelcastRingBufferMultiChannelTest), this should use the ring buffer variant.Apply this diff:
-import com.socketio4j.socketio.store.hazelcast.HazelcastEventStore; +import com.socketio4j.socketio.store.hazelcast_ringbuffer.HazelcastRingBufferEventStore;- return new HazelcastEventStore.Builder(hazelcastPub, hazelcastSub).nodeId(nodeId).build(); + return new HazelcastRingBufferEventStore.Builder(hazelcastPub, hazelcastSub).nodeId(nodeId).build();Also applies to: 54-54
🧹 Nitpick comments (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (2)
19-19: Remove unused import.The
TestInstanceannotation is imported but not applied to the test class.Apply this diff:
-import org.junit.jupiter.api.TestInstance;
29-29: Update outdated comment.The comment references
HazelcastPubSubStorebut should referenceHazelcastRingBufferEventStoreto match the test class name.Apply this diff:
-/** - * Test class for HazelcastPubSubStore using testcontainers - */ +/** + * Test class for HazelcastRingBufferEventStore using testcontainers + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/HazelcastRingBufferEventStoreTest.java (4)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastPubSubSingleChannelUnreliableTest.java (1)
TestInstance(40-140)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (1)
TestInstance(42-139)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java (1)
CustomizedHazelcastContainer(35-84)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastEventStore.java (1)
HazelcastEventStore(41-214)
⏰ 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). (6)
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonReliableEventStoreTest.java (1)
25-49: Test class name does not match the implementation under test.The class is named
RedissonReliableEventStoreTest, but it instantiatesRedissonEventStore(the non-reliable, pub/sub-based implementation) rather thanRedissonReliableEventStore(which usesRReliableTopic).Either rename the test class to
RedissonEventStoreTest, or update the import and instantiation to use the reliable variant:-import com.socketio4j.socketio.store.redis_pubsub.RedissonEventStore; +import com.socketio4j.socketio.store.redis_reliable.RedissonReliableEventStore;- return new RedissonEventStore(redissonPub, redissonSub, EventStoreMode.MULTI_CHANNEL, nodeId); + return new RedissonReliableEventStore.Builder(redissonPub, redissonSub) + .eventStoreMode(EventStoreMode.MULTI_CHANNEL) + .nodeId(nodeId) + .build();netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java (1)
46-68: Test class name mismatch with factory under test.The test class is named
RedissonReliableStoreFactoryTestbut it instantiatesRedissonStoreFactory(lines 66-67), notRedissonReliableStoreFactory.Either rename the test class to match the factory being tested, or update to test the reliable variant:
- return new RedissonStoreFactory(redissonClient, - new RedissonEventStore.Builder(redissonClient).build()); + return new RedissonReliableStoreFactory(redissonClient, + new RedissonReliableEventStore.Builder(redissonClient).build());This would also require updating the imports accordingly.
♻️ Duplicate comments (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableStoreFactory.java (1)
66-69: Missing shutdown of Redisson client - potential resource leak.The
shutdown()method only delegates toeventStore.shutdown()but does not shut down theredissonClient. This could lead to resource leaks if the client is not externally managed. This was flagged in a previous review.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (2)
98-100: User-providedtrimEveryis silently overwritten.When
streamMaxLength != Integer.MAX_VALUE, the user-providedtrimEveryparameter is unconditionally overwritten with a 60-second default. This ignores the caller's explicit configuration.- if (streamMaxLength != Integer.MAX_VALUE) { - trimEvery = Duration.ofSeconds(60); - } + if (streamMaxLength != Integer.MAX_VALUE && trimEvery == null) { + trimEvery = Duration.ofSeconds(60); + log.info("trimEvery defaulted to 60s because streamMaxLength is set"); + }
243-251: Redundantshutdown()call beforeshutdownNow().Line 244 calls
trimExecutor.shutdown()and line 247 immediately callstrimExecutor.shutdownNow()without waiting. Theshutdown()call is redundant becauseshutdownNow()already attempts to stop all actively executing tasks.
🧹 Nitpick comments (10)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
109-109: Remove commented debugging code.This commented
Thread.sleep(600)appears to be a debugging artifact and should be removed to keep the codebase clean.Apply this diff:
- //Thread.sleep(600);netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (1)
109-109: Remove commented debugging code.This commented
Thread.sleep(600)appears to be a debugging artifact and should be removed to keep the codebase clean.Apply this diff:
- //Thread.sleep(600);netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (2)
105-105: Remove commented debugging code.The commented
Thread.sleep(600)should be removed if it's no longer needed for debugging.Apply this diff:
- //Thread.sleep(600);
62-103: Consider extracting common node setup logic.Both node configurations follow an identical pattern (configure, create handlers, start). Extracting this to a helper method would reduce duplication and make the test more maintainable.
Example refactor:
private SocketIOServer createNode(RedissonClient client, EventStoreMode mode) throws Exception { Configuration cfg = new Configuration(); cfg.setHostname("127.0.0.1"); cfg.setPort(findAvailablePort()); cfg.setStoreFactory(new RedissonReliableStoreFactory( client, new RedissonReliableEventStore.Builder(client).eventStoreMode(mode).build() )); SocketIOServer node = new SocketIOServer(cfg); node.addEventListener("join-room", String.class, (c, room, ack) -> { c.joinRoom(room); c.sendEvent("join-ok", "OK"); }); node.addEventListener("leave-room", String.class, (c, room, ack) -> { c.leaveRoom(room); c.sendEvent("leave-ok", "OK"); }); node.start(); return node; }netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (2)
103-103: Remove commented debugging code.The commented
Thread.sleep(600)should be removed if it's no longer needed.Apply this diff:
- //Thread.sleep(600);
62-101: Consider extracting common node setup logic.Like the MultiChannel test, both nodes have identical setup patterns. Consider extracting to a helper method to reduce duplication.
Additionally, since both
DistributedRedissonStreamMultiChannelTestandDistributedRedissonStreamSingleChannelTestshare nearly identical code except for theEventStoreMode, consider creating a shared base class or parameterized test to eliminate cross-file duplication.netty-socketio-core/src/main/java/module-info.java (1)
13-37: Inconsistent indentation in module descriptor.Lines 16-20 and 37 use 4-space indentation while the rest of the file uses 2-space indentation. Consider normalizing for consistency:
exports com.socketio4j.socketio.store.event; exports com.socketio4j.socketio.transport; exports com.socketio4j.socketio.nativeio; - exports com.socketio4j.socketio.store.hazelcast; - exports com.socketio4j.socketio.store.memory; - exports com.socketio4j.socketio.store.redis_pubsub; - exports com.socketio4j.socketio.store.redis_reliable; - requires com.fasterxml.jackson.core; + exports com.socketio4j.socketio.store.hazelcast; + exports com.socketio4j.socketio.store.memory; + exports com.socketio4j.socketio.store.redis_pubsub; + exports com.socketio4j.socketio.store.redis_reliable; + requires com.fasterxml.jackson.core; requires com.fasterxml.jackson.annotation; ... requires org.slf4j; - requires org.jetbrains.annotations; + requires org.jetbrains.annotations;netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableStoreFactory.java (1)
35-35: Unused logger field.The
logfield is declared but never used in this class. Either remove it or add appropriate logging statements.- private static final Logger log = LoggerFactory.getLogger(RedissonReliableStoreFactory.class);netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (2)
109-123: Executor is created unconditionally but only used whentrimEvery != null.The
ScheduledExecutorServiceis always instantiated (lines 109-114), but trimming is only scheduled whentrimEvery != null. IftrimEveryis null, the executor is created but never used, resulting in an idle daemon thread until shutdown.Consider deferring executor creation:
- ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(r -> { - Thread t = new Thread(r, "socketio4j-redis-stream-trimmer"); - t.setDaemon(true); - return t; - }); - this.trimExecutor = executorService; this.trimEvery = trimEvery; if (trimEvery != null) { + ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "socketio4j-redis-stream-trimmer"); + t.setDaemon(true); + return t; + }); + this.trimExecutor = executorService; executorService.scheduleAtFixedRate( this::trimAllReliableStreams, Duration.ofSeconds(1).getSeconds(), trimEvery.getSeconds(), TimeUnit.SECONDS ); + } else { + this.trimExecutor = null; }This would also require a null check in
shutdown0().
201-202: Remove redundant null check ongetReliableTopic()result.
redissonSub.getReliableTopic()returns a client-side proxy object and never returns null—it creates the topic if it doesn't exist. TheObjects.requireNonNullcheck is redundant.RReliableTopic reliableTopic = redissonSub.getReliableTopic(getStreamName(type)); - Objects.requireNonNull(reliableTopic, "reliableTopic can not be null");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableStoreFactory.java(1 hunks)netty-socketio-core/src/main/java/module-info.java(2 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java(4 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonReliableEventStoreTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableStoreFactory.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStore.java (1)
RedissonStore(27-60)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessage.java (1)
EventMessage(21-44)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/event/RedissonReliableEventStoreTest.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonEventStore.java (1)
RedissonEventStore(43-200)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/Configuration.java (1)
Configuration(32-193)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (1)
RedissonReliableEventStore(47-336)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableStoreFactory.java (1)
RedissonReliableStoreFactory(33-77)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/Configuration.java (1)
Configuration(32-193)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (1)
RedissonReliableEventStore(47-336)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableStoreFactory.java (1)
RedissonReliableStoreFactory(33-77)
🪛 ast-grep (0.40.0)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-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). (6)
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: build (25) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (8)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (4)
36-42: LGTM! Well-structured test class setup.The use of
PER_CLASSlifecycle with container reuse is appropriate for integration tests with expensive external dependencies.
45-49: Static analysis warning is a false positive.The
ServerSocket(0)is used solely to obtain an available port number for test server configuration. No network traffic passes through this socket, so the unencrypted socket warning does not apply. The try-with-resources pattern ensures proper cleanup.
108-112: LGTM! Clean configuration helper.Simple and focused helper method for creating Redisson configuration.
114-132: LGTM! Proper resource cleanup.The teardown follows the correct order (nodes → clients → container) and includes appropriate null checks for safety.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (4)
36-42: LGTM! Consistent test structure.The test setup mirrors the MultiChannel variant appropriately. Both tests follow the same pattern, which is good for consistency.
45-49: Static analysis warning is a false positive.Same as the MultiChannel test—
ServerSocket(0)is used only for port discovery, not data transmission.
106-110: LGTM! Clean configuration helper.
112-130: LGTM! Proper resource cleanup.The teardown correctly cleans up resources in the proper order with appropriate null checks.
| } | ||
|
|
||
| // ------------------------------------------- | ||
| // Redis + Node Setup |
There was a problem hiding this comment.
Fix the copy-paste comment referencing Redis.
This comment references "Redis + Node Setup" but this is a Hazelcast test.
Apply this diff:
- // Redis + Node Setup
+ // Hazelcast + Node Setup📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Redis + Node Setup | |
| // Hazelcast + Node Setup |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java
around line 52, the inline comment erroneously reads "Redis + Node Setup";
update that comment to correctly reference Hazelcast (e.g., "Hazelcast + Node
Setup" or simply "Hazelcast Setup") so the comment matches the test's purpose
and remove the misleading Redis reference.
| } | ||
|
|
||
| // ------------------------------------------- | ||
| // Redis + Node Setup |
There was a problem hiding this comment.
Fix the copy-paste comment referencing Redis.
This comment references "Redis + Node Setup" but this is a Hazelcast test.
Apply this diff:
- // Redis + Node Setup
+ // Hazelcast + Node Setup📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Redis + Node Setup | |
| // Hazelcast + Node Setup |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java
around line 52, the inline comment incorrectly reads "Redis + Node Setup" due to
copy-paste; update it to accurately describe the test setup (e.g., "Hazelcast
setup" or "Hazelcast + Node Setup") so the comment reflects Hazelcast usage in
this test. Ensure the replacement matches surrounding comment style and
terminology used in other Hazelcast tests.
| assertNotNull(eventStore, "PubSubStore should not be null"); | ||
| assertTrue(eventStore instanceof RedissonEventStore, "PubSubStore should be RedissonPubSubStore"); |
There was a problem hiding this comment.
Outdated assertion messages reference old naming.
The assertion messages still reference the old "PubSubStore" naming while the code has migrated to "EventStore".
- assertNotNull(eventStore, "PubSubStore should not be null");
- assertTrue(eventStore instanceof RedissonEventStore, "PubSubStore should be RedissonPubSubStore");
+ assertNotNull(eventStore, "EventStore should not be null");
+ assertTrue(eventStore instanceof RedissonEventStore, "EventStore should be RedissonEventStore");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertNotNull(eventStore, "PubSubStore should not be null"); | |
| assertTrue(eventStore instanceof RedissonEventStore, "PubSubStore should be RedissonPubSubStore"); | |
| assertNotNull(eventStore, "EventStore should not be null"); | |
| assertTrue(eventStore instanceof RedissonEventStore, "EventStore should be RedissonEventStore"); |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/RedissonReliableStoreFactoryTest.java
around lines 109 to 110, the test assertion failure messages still reference the
old "PubSubStore" name; update both messages to use the current "EventStore"
terminology (e.g., change "PubSubStore should not be null" to "EventStore should
not be null" and "PubSubStore should be RedissonPubSubStore" to "EventStore
should be RedissonEventStore") so the messages match the migrated class names.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
52-52: Fix the copy-paste comment referencing Redis.This comment incorrectly references "Redis + Node Setup" but this is a Hazelcast test.
Apply this diff:
- // Redis + Node Setup + // Hazelcast + Node Setupnetty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (1)
52-52: Fix the copy-paste comment referencing Redis.This comment incorrectly references "Redis + Node Setup" but this is a Hazelcast test.
Apply this diff:
- // Redis + Node Setup + // Hazelcast + Node Setup
🧹 Nitpick comments (5)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (1)
104-104: Remove or document the commented sleep statement.The commented
Thread.sleep(600)suggests there may have been timing issues during development. Either remove this line if it's no longer needed, or if there's a legitimate race condition, uncomment it and add a comment explaining why the delay is necessary for test reliability.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
109-109: Remove or document the commented-out sleep statement.The commented-out
Thread.sleep(600)should either be removed if no longer needed, or retained with a comment explaining why it's kept for potential future debugging.Apply this diff to remove it:
- //Thread.sleep(600);netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (2)
109-109: Remove or document the commented-out sleep statement.The commented-out
Thread.sleep(600)should either be removed if no longer needed, or retained with a comment explaining why it's kept for potential future debugging.Apply this diff to remove it:
- //Thread.sleep(600);
36-131: Consider consolidating duplicated code between Single and Multi channel tests.This test class is nearly identical to
DistributedHazelcastRingBufferSingleChannelTest.java, differing only in theEventStoreModeparameter. Consider using a parameterized test approach or extracting common setup logic to a shared method to reduce duplication and improve maintainability.For example, you could create a parameterized test:
@ParameterizedTest @EnumSource(EventStoreMode.class) public void testDistributedHazelcastRingBuffer(EventStoreMode mode) throws Exception { // Common setup with mode parameter }Or extract a common setup helper in the base class
DistributedCommonTest.netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (1)
72-79: Consider extracting duplicated event handler registration.Both nodes register identical
join-roomandleave-roomevent handlers. Extracting this logic into a helper method would reduce duplication and improve maintainability.Example refactor:
private void registerRoomHandlers(SocketIOServer server) { server.addEventListener("join-room", String.class, (c, room, ack) -> { c.joinRoom(room); c.sendEvent("join-ok", "OK"); }); server.addEventListener("leave-room", String.class, (c, room, ack) -> { c.leaveRoom(room); c.sendEvent("leave-ok", "OK"); }); }Then call
registerRoomHandlers(node1)andregisterRoomHandlers(node2)in the setup method.Also applies to: 94-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java(1 hunks)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastStoreFactoryTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (3)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (1)
TestInstance(36-132)netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java (1)
CustomizedHazelcastContainer(35-84)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
HazelcastStoreFactory(36-78)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (3)
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java (1)
CustomizedHazelcastContainer(35-84)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (1)
HazelcastStoreFactory(36-78)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast_ringbuffer/HazelcastRingBufferEventStore.java (1)
HazelcastRingBufferEventStore(42-225)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/BaseStoreFactory.java (1)
BaseStoreFactory(37-245)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java (1)
HazelcastStoreFactory(38-130)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_pubsub/RedissonStoreFactory.java (1)
RedissonStoreFactory(33-75)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_reliable/RedissonReliableEventStore.java (1)
RedissonReliableEventStore(47-336)
🪛 ast-grep (0.40.0)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java
[info] 46-46: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java
[info] 45-45: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new ServerSocket(0)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-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). (6)
- GitHub Check: build (25) / build
- GitHub Check: build (17) / build
- GitHub Check: build (21) / build
- GitHub Check: qodana
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (20)
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamSingleChannelTest.java (5)
1-34: LGTM! Well-organized imports and licensing.The Apache 2.0 license header is properly formatted, and all imports are relevant to the Redis Stream single-channel integration test setup.
37-42: LGTM! Appropriate test setup.The use of
PER_CLASSlifecycle and container reuse optimizes the test execution time for this distributed integration test. Separate Redisson clients for each node correctly simulate independent server instances.
46-50: Static analysis warning is a false positive.The unencrypted socket warning from static analysis doesn't apply here.
ServerSocket(0)is used solely to discover an available port number, not for actual network communication. This is a standard pattern in integration tests.Note: There's a minor race condition where the port could be taken between discovery and server startup, but this is an accepted limitation in test environments.
107-111: LGTM! Clean configuration helper.The
redisConfigmethod provides a simple, focused configuration for single-server Redis connections appropriate for this integration test.
114-131: LGTM! Proper resource cleanup.The cleanup method follows best practices with defensive null checks and proper shutdown order (servers → clients → container), ensuring all resources are released correctly even if initialization partially failed.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferSingleChannelTest.java (3)
45-49: ServerSocket usage for port discovery is appropriate for tests.The static analysis tool flags this as an unencrypted socket, but this is a false positive. The ServerSocket is only used to find an available port number and is immediately closed—it's not used for actual network communication.
54-110: Test setup correctly configures SINGLE_CHANNEL mode.The setup properly instantiates two Hazelcast clients and configures both nodes with
EventStoreMode.SINGLE_CHANNEL, which aligns with the test's purpose. The event listener registration and server startup sequence is correct.
113-131: Proper resource cleanup with defensive null checks.The teardown method correctly stops servers, shuts down Hazelcast instances, and stops the container with appropriate null checks, ensuring clean resource release.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedHazelcastRingBufferMultiChannelTest.java (3)
45-49: ServerSocket usage for port discovery is appropriate for tests.The static analysis tool flags this as an unencrypted socket, but this is a false positive. The ServerSocket is only used to find an available port number and is immediately closed—it's not used for actual network communication.
54-110: Test setup correctly configures MULTI_CHANNEL mode.The setup properly instantiates two Hazelcast clients and configures both nodes with
EventStoreMode.MULTI_CHANNEL, which is the key difference from the SingleChannel variant. The event listener registration and server startup sequence is correct.
113-131: Proper resource cleanup with defensive null checks.The teardown method correctly stops servers, shuts down Hazelcast instances, and stops the container with appropriate null checks, ensuring clean resource release.
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java (4)
36-37: LGTM - Appropriate test class structure.The
PER_CLASSlifecycle annotation is correctly used here to allow instance-level setup/teardown methods for managing the Redis container and Redisson clients.
45-49: Port-finding utility is appropriate for local testing.The static analysis warning about unencrypted sockets is a false positive here. This method uses
ServerSocket(0)solely to discover an available port for local test servers—no sensitive data is transmitted. The socket is immediately closed via try-with-resources.
108-112: LGTM - Clean helper method.The
redisConfighelper provides a concise way to create Redisson configuration for single-server mode, appropriate for integration tests.
114-132: LGTM - Comprehensive cleanup with proper null checks.The teardown method properly stops all resources in reverse order with defensive null checks, ensuring clean test isolation even if setup fails partially.
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastStoreFactoryTest.java (2)
56-72: LGTM! Factory setup correctly implements the new EventStore architecture.The test properly configures the Hazelcast client with appropriate settings (
setSmartRouting(false),setRedoOperation(true)) and wires theHazelcastEventStoreusing the builder pattern, aligning with the migration from PubSubStore to EventStore.
110-115: LGTM! Test correctly validates HazelcastEventStore.The assertion for
HazelcastEventStore.classmatches the factory setup at lines 69-71, whereHazelcastEventStore.Builderis used. This differs from the past review comment, which was for a different test file (HazelcastRingBufferStoreFactoryTest) that usesHazelcastRingBufferEventStore.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastStoreFactory.java (3)
48-55: LGTM! Constructor properly validates required dependencies.The constructor correctly enforces non-null constraints on both
hazelcastClientandeventStoreparameters, ensuring the factory is initialized with valid dependencies.
57-60: LGTM! Store and map creation methods correctly delegate to Hazelcast.The
createStore()andcreateMap()methods properly delegate to the underlyinghazelcastClient, and theeventStore()getter correctly exposes the injected EventStore dependency.Also applies to: 68-76
62-65: Clarify the lifecycle ownership model for HazelcastInstance in HazelcastStoreFactory.The
shutdown()method only closes theeventStoreand does not shut down thehazelcastClient. However, this pattern is consistent across all StoreFactory implementations in the codebase—both RedissonStoreFactory and HazelcastStoreFactory follow the same approach of managing only the EventStore lifecycle while leaving the client lifecycle to the caller. This appears to be intentional design: the factory manages only what it creates (EventStore), while the caller who provides the client instance is responsible for managing its shutdown. If this differs from the previous implementation, clarify whether the design pattern should be changed for all StoreFactory implementations or if this represents the intended behavior.
| node2.start(); | ||
| port2 = cfg2.getPort(); | ||
|
|
||
| //Thread.sleep(600); |
There was a problem hiding this comment.
Remove commented-out debug code.
The commented Thread.sleep(600) should be removed. If timing synchronization is genuinely needed, replace it with explicit waits or assertions rather than leaving it as a comment.
Apply this diff:
node2.start();
port2 = cfg2.getPort();
-
- //Thread.sleep(600);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Thread.sleep(600); | |
| node2.start(); | |
| port2 = cfg2.getPort(); | |
| } |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/DistributedRedissonStreamMultiChannelTest.java
around line 105, remove the commented-out debug line "//Thread.sleep(600)"; if
the test actually requires waiting for asynchronous behavior replace the
commented sleep with a proper explicit wait or assertion (e.g., wait for a
condition with a timeout or use existing test utilities to poll until expected
state) rather than leaving commented timing code.
|
|
||
| /** | ||
| * Test class for HazelcastStoreFactory using testcontainers | ||
| * Test class for HazelcastRingBufferStoreFactory using testcontainers |
There was a problem hiding this comment.
Update JavaDoc to reference the correct class name.
The comment mentions "HazelcastRingBufferStoreFactory" but this is the HazelcastStoreFactory test class.
Apply this diff:
-/**
- * Test class for HazelcastRingBufferStoreFactory using testcontainers
- */
+/**
+ * Test class for HazelcastStoreFactory using testcontainers
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Test class for HazelcastRingBufferStoreFactory using testcontainers | |
| /** | |
| * Test class for HazelcastStoreFactory using testcontainers | |
| */ |
🤖 Prompt for AI Agents
In
netty-socketio-core/src/test/java/com/socketio4j/socketio/store/HazelcastStoreFactoryTest.java
around line 47, the JavaDoc comment incorrectly references
"HazelcastRingBufferStoreFactory"; update the comment to reference the correct
class name "HazelcastStoreFactory" (or otherwise reflect this test class) so the
doc accurately matches the tested class.
There was a problem hiding this comment.
Pull request overview
This PR introduces Redis Stream support with single and multi-channel capabilities, renames the PubSub abstraction to EventStore (reflecting that stores can be PubSub/Stream/Broker), and adds comprehensive distributed integration tests. The changes include architectural refactoring with new package structures (event, redis_pubsub, redis_reliable, hazelcast, hazelcast_ringbuffer, memory), introducing reliable/unreliable publishing modes, and enhancing test infrastructure with automatic retry mechanisms.
Key Changes:
- Renamed
PubSubStore→EventStoreacross the codebase with new event-based terminology - Added Redis Stream support via
RedissonReliableEventStorewith single/multi-channel modes - Introduced distributed integration tests covering multiple backend implementations (Redis PubSub, Redis Streams, Hazelcast)
Reviewed changes
Copilot reviewed 75 out of 76 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Added test retry configuration for flaky tests |
| module-info.java | Exported new package structure (event, memory, redis_pubsub, etc.) |
| RedissonReliableEventStore.java | New Redis Stream implementation with reliable delivery |
| RedissonEventStore.java | Unreliable Redis PubSub implementation |
| RedissonStoreFactory.java | Factory for Redis-based stores with EventStore injection |
| HazelcastEventStore.java | Hazelcast PubSub EventStore implementation |
| HazelcastRingBufferEventStore.java | Hazelcast RingBuffer (reliable) EventStore |
| BaseStoreFactory.java | Refactored to support single/multi-channel event subscription |
| DistributedCommonTest.java | Base class for distributed integration tests |
| Various test files | Updated to use EventStore terminology and new package structure |
| Namespace.java | Updated to use EventStore API |
| Configuration.java | Updated JavaDoc references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Type of Change
Related Issue
Closes #39
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.