Double check concurrent logic into simplified one liner, more safe and more performant #59
Conversation
WalkthroughThis change introduces logging infrastructure, adds null-safety validations across storage implementations, expands constructor overloads in factory classes for improved flexibility, and refactors shutdown sequences with ordered cleanup and exception handling in Hazelcast and Redisson store components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@NeatGuyCoding please review this |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonPubSubStore.java (1)
67-74: Guard againstnullregistrations inunsubscribeto avoid NPEIf
unsubscribe(type)is called when there are no registrations for thatPubSubType(or called twice),map.remove(name)returnsnulland thefor (Integer id : regIds)loop will throw aNullPointerException.A small guard keeps this safe without changing normal behavior:
@Override public void unsubscribe(PubSubType type) { String name = type.toString(); - Queue<Integer> regIds = map.remove(name); - RTopic topic = redissonSub.getTopic(name); - for (Integer id : regIds) { - topic.removeListener(id); - } + Queue<Integer> regIds = map.remove(name); + if (regIds == null) { + return; + } + RTopic topic = redissonSub.getTopic(name); + for (Integer id : regIds) { + topic.removeListener(id); + } }This also avoids touching Redisson when there is nothing to unsubscribe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonPubSubStore.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (17) / build
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonPubSubStore.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastPubSubStore.java (1)
70-80: Consider aligning Hazelcast unsubscribe error handling with Redisson
unsubscribe()currently assumestopic.removeMessageListener(id)will always succeed. In the unlikely case Hazelcast throws, a single failure would abort the loop and can leak remaining listeners.You could mirror the defensive pattern used in
RedissonPubSubStore.unsubscribe()by wrappingremoveMessageListenerin a try/catch and logging failures, so all IDs are attempted:- ITopic<Object> topic = hazelcastSub.getTopic(name); - for (UUID id : regIds) { - topic.removeMessageListener(id); - } + ITopic<Object> topic = hazelcastSub.getTopic(name); + for (UUID id : regIds) { + try { + topic.removeMessageListener(id); + } catch (Exception e) { + // keep going even if one removal fails + // log.warn("Failed to remove listener {} from topic {}", id, name, e); + } + }(Adjust logging as desired.)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java (1)
95-115: Ordered, deduplicated Hazelcast shutdown is an improvementUsing a
LinkedHashSetto addhazelcastSub, thenhazelcastPub, thenhazelcastClient, and looping with per‑instance try/catch:
- Avoids double shutdown when the same
HazelcastInstanceis reused for multiple roles.- Ensures a stable, documented shutdown order.
- Logs both successes and failures instead of failing hard on the first exception.
If you ever need
HazelcastPubSubStore.shutdown()semantics as well (topic unsubscribe + internal map clear), you could optionally call it from here before shutting instances down, but the current behavior is already safe and consistent with the Redisson side.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java(1 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastPubSubStore.java(3 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java(3 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonPubSubStore.java(3 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/pubsub/BaseStoreFactory.java (1)
BaseStoreFactory(32-161)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/pubsub/BaseStoreFactory.java (1)
BaseStoreFactory(32-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (9)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastPubSubStore.java (2)
55-67: Subscribe lambda +computeIfAbsentlook correct and thread‑safeUsing
ITopic<T>with a nodeId filter andmap.computeIfAbsent(...).add(regId)removes the previous racy “get or create” pattern and safely tracks listener registrations perPubSubType. This is aligned with the PR’s goal (simpler and safer concurrent logic).
82-87: Shutdown behavior is clean and non‑destructiveUnsubscribing all
PubSubTypevalues and then clearingmapensures internal listener state is cleaned up without touching the underlyingHazelcastInstance, which is appropriate given the factory now owns instance shutdown.netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java (1)
331-333: Good: server now tears down the configured store factory on stopCalling
configCopy.getStoreFactory().shutdown()here completes the lifecycle for Hazelcast/Redisson factories added in this PR and avoids leaking distributed resources when the server stops. The null‑check keeps the behavior safe when no store factory is configured.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java (1)
40-88: Constructors and logging improvements are sensible and safer
- Adding a dedicated
Loggerand usingObjects.requireNonNull(...)in all constructors gives clearer diagnostics for misconfigured Hazelcast instances.- The new 4‑arg constructor that accepts an explicit
HazelcastPubSubStoreprovides a clean injection point for custom pub/sub wiring while still preserving the default wiring in the other constructors.All fields remain
finaland are always initialized along every constructor path, so the class stays immutable after construction.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java (2)
37-97: Constructor overloads and null‑checks clean up configuration paths
- Adding a static
LoggerandObjects.requireNonNullin each constructor makes misconfiguration much easier to spot.- The new overloads—
RedissonStoreFactory(RedissonClient, RedissonPubSubStore)and the 3/4‑argRedissonvariants—give flexibility for custom client/pub/sub wiring and custom pub/sub stores without sacrificing the simple defaultRedisson.create()path.pubSubStoreremainsfinaland non‑null for all constructor paths, preserving invariants.This is a good balance between configurability and safety.
109-129: Ordered Redisson client shutdown is robust and idempotentThe new
shutdown()implementation:
- Uses a
LinkedHashSetwithredisSub,redisPub, thenredisClientto define a clear shutdown order and avoid duplicate shutdown when instances are reused.- Guards each
shutdown()with a null‑check and try/catch, logging both success and failure, so one bad client doesn’t prevent others from stopping.This is a clear upgrade over direct, unguarded shutdown calls.
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonPubSubStore.java (3)
45-64: Constructors now properly validate inputs and wire clientsThe added logger and constructors with
Objects.requireNonNull(...)forRedissonClientarguments ensure that misconfigured pub/sub clients fail fast. The convenienceRedissonPubSubStore(RedissonClient redisson, Long nodeId)cleanly supports the common “single client for all roles” case while the 3‑arg variant supports split pub/sub clients.
73-107: Subscribe/unsubscribe logic is now type‑safe and resilient
subscribe()now passes the providedclazzintotopic.addListener(...)and callslistener.onMessage(msg)directly, eliminating the previous unchecked cast and preserving generic type information.- Using
map.computeIfAbsent(type.toString(), ...)with aConcurrentLinkedQueuegives a concise, thread‑safe way to track registration IDs perPubSubType.unsubscribe()guards againstnull/empty regId queues and wraps eachremoveListener(id)in a try/catch with logging, so a failure to remove one listener doesn’t prevent others from being cleaned up.These changes address the earlier NPE and type‑safety issues and make the pub/sub lifecycle much more robust under concurrency.
110-120: Shutdown cleanly tears down listeners without touching Redis clientsIterating over all
PubSubTypevalues to unsubscribe, then clearing the internalmap, ensures the store’s own listener bookkeeping is reset. Leaving the actualRedissonClientlifecycle to the owning factory avoids double‑shutdown and matches the newRedissonStoreFactory.shutdown()behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastPubSubStore.java (2)
60-72: subscribe(): concurrent registration & self-filtering look good; consider future-proofing null-safetyThe lambda filter and
computeIfAbsent-based registration look correct and thread-safe for concurrent subscriptions. The!nodeId.equals(msg.getMessageObject().getNodeId())check assumes both IDs are always non-null; if that invariant might ever be relaxed, consider guarding with a null-safe comparison (e.g., viaObjects.equals) to avoid a potential NPE. Otherwise this refactor is a clear improvement over manual map handling.
92-96: shutdown(): explicit unsubscribe-all is good; be aware of subscribe vs shutdown raceUnsubscribing all
PubSubTypevalues and then clearing the internal registration map makes the shutdown behavior explicit and avoids touching the Hazelcast client itself (leaving lifecycle to the owning factory), which is appropriate. If there’s any chancesubscribe()can run concurrently with or aftershutdown(), the finalmap.clear()can drop newly added regIds and leave Hazelcast listeners untracked; if that scenario is possible, consider a simple lifecycle guard (e.g., anAtomicBooleanchecked in bothsubscribeandshutdown). If shutdown is only called during full server teardown with no concurrent subscriptions, current logic is fine.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java (3)
49-71: Constructors: null-checks and pubSubStore wiring look sound; minor API-shape questionsThe added
Objects.requireNonNullguards and the new(RedissonClient, RedissonPubSubStore)overload nicely tighten invariants and make dependency injection easier while keepingpubSubStorefinal and non-null. Two optional API-shape thoughts:
- The 2-arg constructor currently accepts
RedissonPubSubStorewhile the field type isPubSubStore; if there’s no Redisson-specific behavior needed here, acceptingPubSubStorewould make the factory more flexible.- For consistency and easier mocking, you might consider using
RedissonClientin all constructor signatures (including the multi-instance variants) unless there’s a concrete-type requirement.
Neither is blocking; the current behavior is correct as written.
72-83: Multi-client constructors: invariants enforced; clarify lifecycle ownership expectationsThe multi-client constructors enforce non-null
redisClient,redisPub,redisSub, andpubSubStore, which is good for catching misconfigurations early. One behavioral nuance: the factory unconditionally owns and shuts down the passed-in clients and pubSubStore inshutdown(). If callers might sometimes pass externally managed Redisson clients or pub/sub stores, it may be worth documenting thatRedissonStoreFactoryassumes ownership of these resources (or introducing a variant that does not shut them down). From a correctness perspective the current setup is internally consistent.Also applies to: 85-97
110-131: shutdown(): ordered, de-duplicated client shutdown is an improvement; consider guarding pubSubStore.shutdown()Calling
pubSubStore.shutdown()first and then shutting downredisSub,redisPub, andredisClientvia an orderedLinkedHashSetis a nice improvement: it enforces sub→pub→client order and naturally deduplicates when all three references point to the same RedissonClient. Since client shutdown is already best-effort with logging, you might similarly wrappubSubStore.shutdown()in a try/catch so a failure there doesn’t prevent client shutdown from running.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastPubSubStore.java(4 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java(3 hunks)netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastStoreFactory.java
🧰 Additional context used
🧬 Code graph analysis (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/pubsub/BaseStoreFactory.java (1)
BaseStoreFactory(32-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (25) / build
- GitHub Check: build (21) / build
- GitHub Check: build (17) / build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/HazelcastPubSubStore.java (1)
75-88: unsubscribe(): per-listener try/catch improves robustnessUsing
map.remove(name)to detach the registration list and then wrapping eachtopic.removeMessageListener(id)in its own try/catch is a solid pattern: one failing removal won’t prevent the rest, and the warning log includes both listener id and topic name for diagnosis. No functional issues spotted here.netty-socketio-core/src/main/java/com/socketio4j/socketio/store/RedissonStoreFactory.java (1)
133-137: createMap(): straightforward delegation to Redisson clientOverriding
createMapto delegate directly toredisClient.getMap(name)aligns with theBaseStoreFactorycontract and keeps map creation centralized on the main client. No issues here.
Description
Double check concurrent into simplified one liner, more safe and more performant
added proper shutdown logic of subscribers
Type of Change
Related Issue
Closes #(issue number)
Changes Made
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.