fix: reconnect breaks async requests

[ttypic]

Resolves #1175

Introduced connect() in AblyBase to reset thread pools when AblyRealtime.connect() is called after close()

Summary by CodeRabbit

• New Features

  • Added explicit connect() capability allowing direct initiation and recovery of networking components so clients can reconnect after closure.

• Tests

  • Added a test that closes a realtime client, reopens it, performs an async time request, and verifies the server time is reasonable, validating reconnection behavior.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds explicit reconnect support for async HTTP by introducing connect() methods that reinitialize the async thread pool after a prior close: AblyRealtime.connect() → AblyBase.connect() → Http.connect() → AsyncHttpScheduler.connect() → CloseableThreadPoolExecutor.connect().

Changes

Cohort / File(s)	Summary
Async scheduler & executor lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java	Adds public void connect() delegating to the inner executor; introduces CloseableThreadPoolExecutor.connect(), makes executor volatile, stores ClientOptions in the executor, switches LinkedBlockingQueue<Runnable>() → LinkedBlockingQueue<>(), and removes finalize()-based recovery.
HTTP delegation lib/src/main/java/io/ably/lib/http/Http.java	Adds public void connect() that delegates to asyncHttp.connect().
Rest base delegation lib/src/main/java/io/ably/lib/rest/AblyBase.java	Adds protected void connect() that delegates to http.connect() for internal use by Realtime client.
Realtime connect order lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java	Marks connect() with @Override and calls super.connect() before connection.connect() to ensure thread-pool reinitialization prior to reconnection.
Reconnection test lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectTest.java	Adds reopened_connection_rest_works() to validate async requests (GET /time) after close() and reconnect, using async callback and time assertion.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant AblyRealtime
    participant AblyBase
    participant Http
    participant AsyncHttpScheduler
    participant CloseableThreadPoolExecutor

    Client->>AblyRealtime: connect()
    activate AblyRealtime
    AblyRealtime->>AblyBase: super.connect()
    activate AblyBase
    AblyBase->>Http: connect()
    activate Http
    Http->>AsyncHttpScheduler: connect()
    activate AsyncHttpScheduler
    AsyncHttpScheduler->>CloseableThreadPoolExecutor: connect()
    activate CloseableThreadPoolExecutor
    Note over CloseableThreadPoolExecutor: Recreate thread pool if shutdown
    CloseableThreadPoolExecutor-->>AsyncHttpScheduler: ready
    deactivate CloseableThreadPoolExecutor
    AsyncHttpScheduler-->>Http: ready
    deactivate AsyncHttpScheduler
    Http-->>AblyBase: ready
    deactivate Http
    AblyBase-->>AblyRealtime: ready
    deactivate AblyBase
    AblyRealtime->>AblyRealtime: connection.connect()
    AblyRealtime-->>Client: connected
    deactivate AblyRealtime

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review CloseableThreadPoolExecutor.connect() thread-safety and race conditions around the volatile executor.
• Ensure ClientOptions are used correctly when recreating the pool and no lifecycle/resource leaks remain.
• Verify AblyRealtime.connect() ordering and that tests exercise edge cases (concurrent close/connect and pending async tasks).

Poem

🐰
When pools slept after a close, I peered,
I nudged connect(), the threads reappeared,
Requests now dance and hop in line,
Timely ticks and responses fine,
A carrot-cheered reconnect — all clear! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix: reconnect breaks async requests" is concise, clear, and directly summarizes the main objective of this changeset. The title accurately reflects the primary problem being addressed: fixing the issue where asynchronous requests stop working after calling close() followed by connect(). The changes across multiple files all serve this singular purpose by adding connect() methods to reinitialize the thread pool during reconnection, making the title appropriately scoped and specific to the core issue.
Linked Issues Check	✅ Passed	The code changes directly address all objectives from the linked issues (#1175 and ECO-5626). The pull request implements thread pool reinitialization by adding connect() methods across the HTTP and realtime client hierarchy (AsyncHttpScheduler, Http, AblyBase, and AblyRealtime), with AblyRealtime.connect() now calling super.connect() to trigger the reinitialization chain. The addition of the reopened_connection_rest_works() test method validates that async requests function correctly after a reconnect cycle, confirming the fix resolves the problem where close() followed by connect() left the client unable to perform asynchronous operations.
Out of Scope Changes Check	✅ Passed	All code changes in this pull request are directly scoped to the fix for reconnect breaking async requests. The connect() methods added to AsyncHttpScheduler, Http, AblyBase, and AblyRealtime, along with supporting infrastructure changes (volatile executor field, options field storage), all serve the purpose of reinitializing the thread pool during reconnection. The replacement of finalize()-based cleanup with explicit connect() handling is a logical architectural improvement supporting the same fix, and the test method specifically validates the reconnect scenario. No extraneous or tangential changes are present.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ECO-5626/fix-reconnect

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)

69-79: Consider synchronization to prevent race condition.

The check-then-act pattern (line 70: check isShutdown(), lines 71-77: create new executor) is not atomic. If multiple threads call connect() simultaneously after close(), both could create new executors, with the second overwriting the first, leaking resources.

While unlikely in the expected single-threaded usage (close → connect), adding synchronized would eliminate the risk with minimal overhead.

Consider this change:

-        public void connect() {
+        public synchronized void connect() {
             if (executor.isShutdown()) {
                 executor = new ThreadPoolExecutor(

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 6d66f58 and 764e71c.

📒 Files selected for processing (5)

• lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (2 hunks)
• lib/src/main/java/io/ably/lib/http/Http.java (1 hunks)
• lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1 hunks)
• lib/src/main/java/io/ably/lib/rest/AblyBase.java (1 hunks)
• lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectTest.java (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectTest.java (3)

lib/src/test/java/io/ably/lib/test/common/Helpers.java (1)

• Helpers (74-1229)

lib/src/main/java/io/ably/lib/types/AsyncHttpPaginatedResponse.java (1)

• AsyncHttpPaginatedResponse (5-44)

lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)

• ErrorInfo (17-197)

⏰ 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
• GitHub Check: check-liveobjects
• GitHub Check: check-realtime-okhttp
• GitHub Check: check (29)
• GitHub Check: check-rest-okhttp
• GitHub Check: check (29)
• GitHub Check: check-rest
• GitHub Check: check-realtime
• GitHub Check: check (19)
• GitHub Check: check (24)
• GitHub Check: check (21)

🔇 Additional comments (5)

lib/src/main/java/io/ably/lib/http/Http.java (1)

24-26: LGTM! Clean delegation to async HTTP scheduler.

The method correctly delegates to asyncHttp.connect() to reinitialize the thread pool after close.

lib/src/main/java/io/ably/lib/rest/AblyBase.java (1)

141-146: LGTM! Appropriate internal API for realtime client.

The protected visibility and clear documentation make this a clean internal extension point for AblyRealtime.

lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)

99-102: LGTM! Correct sequencing for thread pool recovery.

Calling super.connect() before connection.connect() ensures the async HTTP thread pool is restored before establishing the WebSocket connection. This addresses the core issue where async operations fail after reconnect.

lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (2)

36-38: LGTM! Clean delegation pattern.

---

41-54: Good defensive design with volatile executor field.

Making executor volatile ensures visibility across threads, and storing options enables correct thread pool recreation. The comment at line 41 helpfully explains the rationale.

[Copilot]

Pull Request Overview

This PR fixes an issue where REST API requests fail after a realtime connection is reopened (closed then reconnected). The fix ensures the HTTP thread pool is properly reinitialized when connect() is called after a previous close().

Key changes:

• Added a connect() method chain through AblyRealtime → AblyBase → Http → AsyncHttpScheduler to reinitialize the thread pool
• Modified CloseableThreadPoolExecutor to check if the executor is shutdown and create a new one if needed
• Added test coverage for the reopened connection REST functionality scenario

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java	Overrides connect() to call parent method for HTTP thread pool reset
lib/src/main/java/io/ably/lib/rest/AblyBase.java	Adds protected connect() method to reinitialize HTTP layer
lib/src/main/java/io/ably/lib/http/Http.java	Delegates connect() call to async HTTP scheduler
lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java	Implements thread pool reinitialization logic and removes deprecated finalize method
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectTest.java	Adds test case verifying REST requests work after connection is reopened

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)

69-79: Consider guarding against multiple connect() calls and resource leaks.

The method only recreates the executor if it's shutdown, which is correct. However:

1. Race condition (as noted in previous review): Between checking isShutdown() on line 70 and creating the new executor on line 71, another thread could call execute() on the shutdown executor, resulting in a RejectedExecutionException. While the developer noted this is the user's responsibility, consider documenting this threading requirement clearly.

2. Idempotency consideration: If connect() is called when the executor is NOT shutdown, nothing happens. This is likely intentional but should be documented. Alternatively, consider logging a warning if connect() is called on a non-shutdown executor to help users debug unexpected behavior.

 public void connect() {
     if (executor.isShutdown()) {
         executor = new ThreadPoolExecutor(
             options.asyncHttpThreadpoolSize,
             options.asyncHttpThreadpoolSize,
             KEEP_ALIVE_TIME,
             TimeUnit.MILLISECONDS,
             new LinkedBlockingQueue<>()
         );
+    } else {
+        Log.w(TAG, "connect() called on non-shutdown executor; ignoring");
     }
 }

🧹 Nitpick comments (2)

lib/src/main/java/io/ably/lib/rest/AblyBase.java (1)

141-146: Enhance documentation and consider idempotency.

The JavaDoc comment should follow the standard format used by other methods in this class and explain:

• When this method should be called (e.g., after close())
• What it does (reinitializes the async thread pool)
• Whether it's safe to call multiple times
• The relationship to close() and the connection lifecycle

Consider adding a check to make this method idempotent—currently, calling connect() multiple times could have unclear behavior depending on the underlying http.connect() implementation.

lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)

36-38: Add JavaDoc for the public connect() method.

This method is part of the public API but lacks documentation. Add JavaDoc explaining:

• Its purpose (reinitializes the async thread pool after shutdown)
• When it should be called (after close() or when the executor has been shut down)
• Whether it's safe to call multiple times
• Any threading considerations

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 764e71c and 6e8a5a6.

📒 Files selected for processing (5)

• lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (2 hunks)
• lib/src/main/java/io/ably/lib/http/Http.java (1 hunks)
• lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1 hunks)
• lib/src/main/java/io/ably/lib/rest/AblyBase.java (1 hunks)
• lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectTest.java (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• lib/src/main/java/io/ably/lib/http/Http.java
• lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectTest.java
• lib/src/main/java/io/ably/lib/realtime/AblyRealtime.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). (2)

• GitHub Check: check-realtime-okhttp
• GitHub Check: check-realtime

🔇 Additional comments (1)

lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)

41-43: Volatile ensures visibility but not atomicity.

Good use of volatile for the executor field to ensure visibility of the reference across threads. However, note that this doesn't prevent race conditions between connect() and execute() or between multiple connect() calls—it only ensures threads see the most recent reference.

The addition of the options field enables recreating the executor with the correct configuration.

[sacOO7]

lgtm