[ECO-5514] fix: improve WebSocket transport lifecycle and activity management #1165
Conversation
WalkthroughDecouples WebSocket client lifecycle via a new WebSocketHandler, enforces single connect invocation, refines activity-timer synchronization and error handling, adds ConnectionManager passthroughs, a reflection test helper, and new unit tests for connect/close and activity-timer behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant T as WebSocketTransport
participant H as WebSocketHandler
participant C as WebSocketClient
participant Timer as ActivityTimer
App->>T: connect()
T->>H: instantiate & wire handler
T->>C: open(url, handler)
C-->>H: onOpen/onMessage/onClose/onError
H->>H: flagActivity()
H->>Timer: check/start/reschedule (guarded)
Timer-->>H: expiry -> onActivityTimerExpiry()
alt idle timeout exceeded
H->>C: cancel/close (ABNORMAL_CLOSE)
else activity observed
H->>Timer: reschedule with updated timeout
end
App->>T: close()
T->>T: snapshot client & handler
alt handler & client non-null
T->>H: flagActivity()/dispose()
T->>C: close()
else
T->>T: log uninitialized/already-closed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-25T12:18:35.432ZApplied to files:
⏰ 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). (12)
🔇 Additional comments (10)
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 |
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 (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (1)
114-124: Dispose handler on connect failures to prevent Timer thread leaks
new WebSocketHandler()creates a Timer thread immediately. Ifcreate()orconnect()throws, the handler isn’t disposed and the Timer thread leaks. Dispose and null out on both catch paths.} catch (AblyException e) { Log.e(TAG, "Unexpected exception attempting connection; wsUri = " + wsUri, e); connectListener.onTransportUnavailable(this, e.errorInfo); + if (webSocketHandler != null) { + webSocketHandler.dispose(); + webSocketHandler = null; + } + webSocketClient = null; } catch (Throwable t) { Log.e(TAG, "Unexpected exception attempting connection; wsUri = " + wsUri, t); connectListener.onTransportUnavailable(this, AblyException.fromThrowable(t).errorInfo); + if (webSocketHandler != null) { + webSocketHandler.dispose(); + webSocketHandler = null; + } + webSocketClient = null; }
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (3)
238-238: Use a daemon Timer with a descriptive nameNon-daemon Timers can pin the JVM; naming aids debugging. This reduces impact if disposal is delayed.
- private final Timer timer = new Timer(); + private final Timer timer = new Timer("Ably-WS-activity", true);
347-349: Harden dispose: clear task under monitor and purge Timer queuePrevents stale
activityTimerTaskfrom blocking future scheduling and cleans the Timer queue.- private void dispose() { - timer.cancel(); - } + private void dispose() { + synchronized (activityTimerMonitor) { + activityTimerTask = null; + } + timer.cancel(); + timer.purge(); + }
399-406: Reset task on schedule failure and null-check before cancelIf scheduling fails (e.g., Timer already cancelled), clear
activityTimerTaskso future attempts can reschedule; also guard againstwebSocketClientbeing null.private void schedule(TimerTask task, long delay) { try { timer.schedule(task, delay); } catch (IllegalStateException ise) { Log.e(TAG, "Unexpected exception scheduling activity timer", ise); - webSocketClient.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + } + synchronized (activityTimerMonitor) { + if (activityTimerTask == task) { + activityTimerTask = null; + } + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (1)
lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)
⏰ 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). (12)
- GitHub Check: check
- GitHub Check: check-rest-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (29)
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: build
🔇 Additional comments (3)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (3)
351-366: LGTM: activity check gating and early returns are clearThe guard for
maxIdleInterval == 0and test toggle is sound; delegating tocheckActivity()keeps flow simple.
368-383: LGTM: fine-grained locking with a dedicated monitorLimiting synchronization to the timer start path avoids global locks and mitigates deadlocks.
419-425: LGTM: expiry path uses monitor and restarts safelyNulling
activityTimerTaskunder the monitor before rescheduling avoids double-starts.
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java
Outdated
Show resolved
Hide resolved
0c75dbc to
7d8e7b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (4)
116-126: Dispose handler on connection failure to prevent Timer thread leak.If engine.create/connect throws after handler construction, its Timer thread remains unless disposed.
} catch (AblyException e) { Log.e(TAG, "Unexpected exception attempting connection; wsUri = " + wsUri, e); + final WebSocketHandler h = webSocketHandler; + if (h != null) { h.dispose(); } connectListener.onTransportUnavailable(this, e.errorInfo); } catch (Throwable t) { Log.e(TAG, "Unexpected exception attempting connection; wsUri = " + wsUri, t); + final WebSocketHandler h = webSocketHandler; + if (h != null) { h.dispose(); } connectListener.onTransportUnavailable(this, AblyException.fromThrowable(t).errorInfo); }
269-280: Handle non-array-backed ByteBuffers to avoid UnsupportedOperationException.blob.array() is unsafe for direct/read-only buffers and your catch only covers AblyException.
- try { - ProtocolMessage msg = ProtocolSerializer.readMsgpack(blob.array()); + try { + final byte[] data; + if (blob.hasArray()) { + int len = blob.remaining(); + int off = blob.arrayOffset() + blob.position(); + byte[] src = blob.array(); + if (off == 0 && len == src.length) { + data = src; + } else { + data = new byte[len]; + System.arraycopy(src, off, data, 0, len); + } + } else { + int len = blob.remaining(); + data = new byte[len]; + blob.slice().get(data); + } + ProtocolMessage msg = ProtocolSerializer.readMsgpack(data);
418-422: Null-check client before force-cancel on timeout.Defensive against concurrent disposal.
- webSocketClient.cancel(ABNORMAL_CLOSE, "timed out"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "timed out"); + }
304-305: Minor: remove misleading ‘remote = false’ from log.No remote flag is tracked here; logging a hardcoded false is confusing.
- Log.d(TAG, "onClose(): wsCode = " + wsCode + "; wsReason = " + wsReason + "; remote = " + false); + Log.d(TAG, "onClose(): wsCode = " + wsCode + "; wsReason = " + wsReason);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (1)
lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)
⏰ 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). (12)
- GitHub Check: build
- GitHub Check: check (29)
- GitHub Check: check-rest
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check-liveobjects
- GitHub Check: check
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest-okhttp
🔇 Additional comments (5)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (5)
129-135: Good guard to enforce single connect call.
383-389: LGTM on fine-grained monitor usage for the activity timer.This avoids broad transport locking and reduces deadlock risk.
52-56: Make shared refs volatile for correct cross-thread visibility (client/handler/connectListener).These fields are written in connect() and read in handler/close threads. Without volatile, other threads may see stale nulls or older instances.
- private ConnectListener connectListener; - private WebSocketClient webSocketClient; + private volatile ConnectListener connectListener; + private volatile WebSocketClient webSocketClient; private final WebSocketEngine webSocketEngine; - private WebSocketHandler webSocketHandler; + private volatile WebSocketHandler webSocketHandler;Optionally also make the test switch volatile:
- private boolean activityCheckTurnedOff = false; + private volatile boolean activityCheckTurnedOff = false;
140-159: Also dispose handler when client is null to avoid stray timer thread.} else { Log.w(TAG, "close() called on uninitialized or already closed transport"); + if (handler != null) { + handler.dispose(); + } }
392-404: Avoid NPE when canceling from timer catch; catch Throwable.onClose may null or dispose client while the task runs. Snapshot and null-check.
- } catch (Exception exception) { - Log.e(TAG, "Unexpected exception in activity timer handler", exception); - webSocketClient.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + } catch (Throwable t) { + Log.e(TAG, "Unexpected exception in activity timer handler", t); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + } }
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(2 hunks)lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java(7 hunks)lib/src/test/java/io/ably/lib/test/common/Helpers.java(1 hunks)lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java
🧰 Additional context used
🧬 Code graph analysis (3)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)
lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java (4)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
Helpers(74-1229)lib/src/test/java/io/ably/lib/test/util/EmptyPlatformAgentProvider.java (1)
EmptyPlatformAgentProvider(5-10)lib/src/main/java/io/ably/lib/transport/ITransport.java (1)
TransportParams(37-92)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
setPrivateField(22-26)
⏰ 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). (12)
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check-realtime
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (29)
🔇 Additional comments (1)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
861-866: Expose cached auth query params to transports.Handy passthrough to
ably.auth.getAuthParams()so transports can reuse the connection manager’s current credentials. Nothing else about state management changes here, so no concerns.
…nagement * Force-cancel hanging socket connections * Replace synchronized blocks with fine-grained locking using `activityTimerMonitor` to avoid deadlocks. * Refactor `close()` for safer access to shared fields and handle uninitialized cases gracefully. * Simplify activity timer logic and ensure consistent disposal of `WebSocketClient` and `WebSocketHandler`.
24ae90e to
620fdf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (2)
100-128: Dispose handler on connect() failure to avoid Timer thread leaksIf create(...) or connect() throws after the handler is created, its Timer thread survives. Dispose it in the catch blocks.
} catch (AblyException e) { Log.e(TAG, "Unexpected exception attempting connection; wsUri = " + wsUri, e); connectListener.onTransportUnavailable(this, e.errorInfo); + if (webSocketHandler != null) { + webSocketHandler.dispose(); + } } catch (Throwable t) { Log.e(TAG, "Unexpected exception attempting connection; wsUri = " + wsUri, t); connectListener.onTransportUnavailable(this, AblyException.fromThrowable(t).errorInfo); + if (webSocketHandler != null) { + webSocketHandler.dispose(); + } }
422-430: Also null-check client on timeout cancellationAvoid NPE in rare paths where handler exists but client creation failed.
- Log.e(TAG, "No activity for " + getActivityTimeout() + "ms, closing connection"); - webSocketClient.cancel(ABNORMAL_CLOSE, "timed out"); + Log.e(TAG, "No activity for " + getActivityTimeout() + "ms, closing connection"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "timed out"); + } return;
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (2)
52-53: Mark connectListener volatile for safe publication to WS callback threadsWS events may fire on different threads immediately after connect(); volatile ensures visibility.
- private ConnectListener connectListener; + private volatile ConnectListener connectListener;
168-195: Snapshot client reference in send() to avoid races with close()Use a local final reference and fail fast if null; route through existing NotConnectedException path.
public void send(ProtocolMessage msg) throws AblyException { Log.d(TAG, "send(); action = " + msg.action); try { + final WebSocketClient c = this.webSocketClient; + if (c == null) { + throw new NotConnectedException(); + } if (channelBinaryMode) { byte[] encodedMsg = ProtocolSerializer.writeMsgpack(msg); // Check the logging level to avoid performance hit associated with building the message if (Log.level <= Log.VERBOSE) { ProtocolMessage decodedMsg = ProtocolSerializer.readMsgpack(encodedMsg); Log.v(TAG, "send(): " + decodedMsg.action + ": " + new String(ProtocolSerializer.writeJSON(decodedMsg))); } - webSocketClient.send(encodedMsg); + c.send(encodedMsg); } else { // Check the logging level to avoid performance hit associated with building the message if (Log.level <= Log.VERBOSE) Log.v(TAG, "send(): " + new String(ProtocolSerializer.writeJSON(msg))); - webSocketClient.send(ProtocolSerializer.writeJSON(msg)); + c.send(ProtocolSerializer.writeJSON(msg)); } } catch (NotConnectedException e) { if (connectListener != null) { connectListener.onTransportUnavailable(this, AblyException.fromThrowable(e).errorInfo); } else throw AblyException.fromThrowable(e); } catch (Exception e) { throw AblyException.fromThrowable(e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(2 hunks)lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java(8 hunks)lib/src/test/java/io/ably/lib/test/common/Helpers.java(1 hunks)lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java
- lib/src/main/java/io/ably/lib/transport/ConnectionManager.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T12:18:35.402Z
Learnt from: ttypic
PR: ably/ably-java#1165
File: lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java:250-251
Timestamp: 2025-09-25T12:18:35.402Z
Learning: In lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java, the lastActivityTime field is accessed from multiple threads (WebSocket callback threads and Timer thread) without synchronization, requiring volatile for proper visibility guarantees.
Applied to files:
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java
🧬 Code graph analysis (1)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
liveobjects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
setPrivateField(22-26)
⏰ 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). (12)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check-rest
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-realtime-okhttp
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (29)
🔇 Additional comments (10)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
972-980: Extend reflection helper to walk superclasses to avoid failures when field is inheritedCurrent implementation fails for fields declared on a superclass. Walk the class hierarchy before giving up.
- public static void setPrivateField(Object object, String fieldName, Object value) { - try { - Field connectionStateField = object.getClass().getDeclaredField(fieldName); - connectionStateField.setAccessible(true); - connectionStateField.set(object, value); - } catch (Exception e) { - fail("Failed accessing " + fieldName + " with error " + e); - } - } + public static void setPrivateField(Object object, String fieldName, Object value) { + for (Class<?> type = object.getClass(); type != null; type = type.getSuperclass()) { + try { + Field field = type.getDeclaredField(fieldName); + field.setAccessible(true); + field.set(object, value); + return; + } catch (NoSuchFieldException ignored) { + // keep walking + } catch (Exception e) { + fail("Failed accessing " + fieldName + " with error " + e); + return; + } + } + fail("Field \"" + fieldName + "\" not found on " + object.getClass()); + }lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (9)
53-56: Make shared refs volatile to restore memory-visibility guarantees- private WebSocketClient webSocketClient; + private volatile WebSocketClient webSocketClient; private final WebSocketEngine webSocketEngine; - private WebSocketHandler webSocketHandler; + private volatile WebSocketHandler webSocketHandler;
130-137: Good guard to enforce single connect callSynchronized one-time guard prevents lifecycle misuse and leaks.
141-160: Also dispose handler when client is null to avoid stray Timer threadsIf client is null but handler exists, cancel the handler’s Timer.
} else { Log.w(TAG, "close() called on uninitialized or already closed transport"); + if (handler != null) { + handler.dispose(); + } }
224-227: Nice: explicit active-transport check helperEncapsulating this via ConnectionManager avoids tight coupling in tests and production code.
254-254: Make activity Timer a daemon (and name it) to avoid pinning JVM if disposal is missedDaemon timer reduces impact of missed cancel() and aids diagnostics.
- private final Timer timer = new Timer(); + private final Timer timer = new Timer("WebSocketActivity-" + System.identityHashCode(WebSocketTransport.this), true);
256-257: lastActivityTime needs volatile for visibility between WS callback threads and Timer thread- private long lastActivityTime; + private volatile long lastActivityTime;
360-362: LGTM: handler dispose is simple and idempotentCanceling the Timer is sufficient; schedule() already tolerates canceled timers.
364-381: LGTM: activity gating only updates timestamp when active, but still arms timerThis keeps ConnectionManager metrics clean while still enabling force-cancel on close.
400-412: Null-deref risk when canceling client from TimerTask catchwebSocketClient can be nulled or not yet created; snapshot and null-check before canceling.
- webSocketClient.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + }
- Test edge cases like multiple `connect()` calls, forced closures, and redundant `onClose` events.
620fdf0 to
a6f0f75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java (1)
1-154: Replace deprecated invocation.getArgumentAt calls
Use invocation.getArgument(index) instead of getArgumentAt(…) across tests:
- lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java (lines 83, 116, 140)
- lib/src/test/java/io/ably/lib/http/HttpHelpersTest.java (lines 31, 52)
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
868-877: Decouple from concrete transport: accept ITransport instead of WebSocketTransportAvoids unnecessary coupling from ConnectionManager to a concrete transport type.
Apply:
- boolean isActiveTransport(WebSocketTransport transport) { + boolean isActiveTransport(ITransport transport) { return transport == this.transport; }lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java (1)
1-154: Optional: tighten matcher typing for ErrorInfoTo avoid raw any() ambiguity, prefer any(ErrorInfo.class) in verifications.
Example:
- verify(connectListener).onTransportUnavailable(eq(transport), any()); + verify(connectListener).onTransportUnavailable(eq(transport), any(ErrorInfo.class));lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (1)
256-256: Make lastActivityTime volatile for cross-thread visibilityWritten by WS callback threads, read by Timer thread; volatile avoids stale reads.
- private long lastActivityTime; + private volatile long lastActivityTime;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(2 hunks)lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java(8 hunks)lib/src/test/java/io/ably/lib/test/common/Helpers.java(1 hunks)lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/test/java/io/ably/lib/test/common/Helpers.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T12:18:35.432Z
Learnt from: ttypic
PR: ably/ably-java#1165
File: lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java:250-251
Timestamp: 2025-09-25T12:18:35.432Z
Learning: In lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java, the lastActivityTime field is accessed from multiple threads (WebSocket callback threads and Timer thread) without synchronization, requiring volatile for proper visibility guarantees.
Applied to files:
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java
🧬 Code graph analysis (2)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)
lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java (5)
lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)
Helpers(74-1229)lib/src/test/java/io/ably/lib/test/util/EmptyPlatformAgentProvider.java (1)
EmptyPlatformAgentProvider(5-10)lib/src/main/java/io/ably/lib/transport/ITransport.java (1)
TransportParams(37-92)lib/src/main/java/io/ably/lib/types/ClientOptions.java (1)
ClientOptions(19-392)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)
⏰ 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). (12)
- GitHub Check: build
- GitHub Check: check-realtime
- GitHub Check: check-rest-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: check
🔇 Additional comments (13)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
34-34: LGTM: Param importImport is required for getAuthParams(). No issues.
861-867: LGTM: add getAuthParams() passthroughPackage-private accessor is appropriate for transport usage and tests. Exception propagation is correct.
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (7)
100-128: LGTM: connect flow and single-call guardEnforcing single connect, building wsUri with auth/connect params, and robust exception handling looks good.
133-136: LGTM: connect-once enforcementSynchronized guard prevents leaks from double connect.
254-254: Make Timer daemon (or lazy-init) to reduce leak impactNon-daemon Timer can keep the process alive if disposal is missed.
- private final Timer timer = new Timer(); + private final Timer timer = new Timer("WebSocketActivity-" + System.identityHashCode(WebSocketTransport.this), true);
400-412: Null-check client before cancel in TimerTask catchwebSocketClient may be null if close raced; prevent NPE.
- webSocketClient.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + }
364-381: LGTM: activity gating and timer armingOnly flags activity when this is the active transport; respects maxIdleInterval and test override.
383-399: LGTM: start timer once, let expiry rescheduleMinimizes contention and avoids frequent rescheduling.
141-161: Dispose handler when client is null to avoid Timer thread leakIf close() races before client assignment (handler already created), else branch logs but leaves handler Timer running. That Timer is non-daemon and can pin the JVM.
Apply:
} else { Log.w(TAG, "close() called on uninitialized or already closed transport"); + if (handler != null) { + handler.dispose(); + } }lib/src/test/java/io/ably/lib/transport/WebSocketTransportTest.java (4)
82-85: Replace removed Mockito API getArgumentAt with getArgumentMockito 3+ removed getArgumentAt; current code won’t compile.
Apply:
- when(mockEngine.create(any(), any())).thenAnswer(invocation -> { - webSocketListenerRef.set(invocation.getArgumentAt(1, WebSocketListener.class)); + when(mockEngine.create(any(), any())).thenAnswer(invocation -> { + webSocketListenerRef.set(invocation.getArgument(1, WebSocketListener.class)); return mockWebSocketClient; });
87-94: Ditto: use getArgument(...) to forward cancel argsSame compilation issue here.
- doAnswer(invocation -> { - webSocketListenerRef.get().onClose( - invocation.getArgumentAt(0, Integer.class), - invocation.getArgumentAt(1, String.class) - ); - return null; - }).when(mockWebSocketClient).cancel(anyInt(), anyString()); + doAnswer(invocation -> { + webSocketListenerRef.get().onClose( + invocation.getArgument(0, Integer.class), + invocation.getArgument(1, String.class) + ); + return null; + }).when(mockWebSocketClient).cancel(anyInt(), anyString());
115-118: Replace getArgumentAt in listener captureUpdate to supported Mockito API.
- when(mockEngine.create(any(), any())).thenAnswer(invocation -> { - listenerRef.set(invocation.getArgumentAt(1, WebSocketListener.class)); + when(mockEngine.create(any(), any())).thenAnswer(invocation -> { + listenerRef.set(invocation.getArgument(1, WebSocketListener.class)); return mockWebSocketClient; });
139-142: Last getArgumentAt occurrenceUpdate to getArgument here as well.
- when(mockEngine.create(any(), any())).thenAnswer(invocation -> { - listenerRef.set(invocation.getArgumentAt(1, WebSocketListener.class)); + when(mockEngine.create(any(), any())).thenAnswer(invocation -> { + listenerRef.set(invocation.getArgument(1, WebSocketListener.class)); return mockWebSocketClient; });
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)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (1)
431-433: Guard against races when force-canceling on timeoutSame race applies here; null-check before calling
cancel(...)to avoid Timer thread exceptions.- webSocketClient.cancel(ABNORMAL_CLOSE, "timed out"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "timed out"); + }
🧹 Nitpick comments (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (1)
364-381: Prefer monotonic clock for elapsed-time checksUsing
System.currentTimeMillis()for elapsed time can be skewed by clock adjustments. Consider switching toSystem.nanoTime()for computing inactivity intervals to avoid false positives/negatives.Also applies to: 425-431
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T12:18:35.432Z
Learnt from: ttypic
PR: ably/ably-java#1165
File: lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java:250-251
Timestamp: 2025-09-25T12:18:35.432Z
Learning: In lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java, the lastActivityTime field is accessed from multiple threads (WebSocket callback threads and Timer thread) without synchronization, requiring volatile for proper visibility guarantees.
Applied to files:
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java
🧬 Code graph analysis (1)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (2)
lib/src/main/java/io/ably/lib/http/HttpUtils.java (1)
HttpUtils(28-280)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)
⏰ 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). (12)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (7)
lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java (7)
255-257: Good: volatile fields restore cross-thread visibilityMarking
activityTimerTaskandlastActivityTimeas volatile ensures visibility between WS callback threads and the Timer thread. This aligns with concurrency best practices.Based on learnings
133-136: Single-connect guard looks goodThe synchronized guard is simple and effective to prevent multiple connect calls.
224-226: Helper to check active transport is a good additionThis improves coordination with ConnectionManager and activity logic.
444-446: Confirm activity-timeout source change is intentionalUsing
params.options.realtimeRequestTimeout(instead of a manager-level setting) modifies the effective timeout. Please confirm this matches spec/intent for reconnection vs idle detection timing.
409-411: Avoid NPE when canceling from TimerTask catch block
webSocketClientcan be concurrently null or not yet initialized when the catch runs; dereferencing it risks NPE. Snapshot or null-check first.- webSocketClient.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + final WebSocketClient c = webSocketClient; + if (c != null) { + c.cancel(ABNORMAL_CLOSE, "Activity timer closed unexpectedly"); + }
254-254: Make the Timer a daemon (and name it) to avoid pinning the JVM
new Timer()creates a non-daemon thread. If disposal is missed in any edge case, it can keep the JVM alive. Use a daemon timer and give it a helpful name.- private final Timer timer = new Timer(); + private final Timer timer = new Timer("WebSocketActivity-" + System.identityHashCode(WebSocketTransport.this), true);
151-160: Dispose handler when client is null to avoid Timer thread leakIf
webSocketClient == nullbutwebSocketHandler != null,close()only logs and leaves the handler’s Timer thread running. This leaks a non-daemon thread and undermines the PR’s objective to prevent leaks.Apply:
} else { Log.w(TAG, "close() called on uninitialized or already closed transport"); + if (handler != null) { + handler.dispose(); + } }
…tTransport - marked `lastActivityTime` as volatile, it's low cost, but it will ensure that the timer reads fresh value, otherwise it can read very old value (although it's very unlikely) - check activity timer before going to the synchronized block
718573c to
f14844a
Compare
Resolves #1144
activityTimerMonitorto avoid deadlocks.close()for safer access to shared fields and handle uninitialized cases gracefully.WebSocketClientandWebSocketHandler.Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests