Patch for Github issue #524 #526
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances the shutdown reliability of the AnalyticsClient by preventing race conditions and ensuring proper cleanup of background tasks. The changes introduce explicit tracking of the looper thread, add a wait mechanism to ensure message processing completes before shutdown, and handle cases where batch submission fails during shutdown.
Key Changes
- Added
looperFuturetracking to wait for looper completion before shutting down executors (lines 72, 136-138, 241-257) - Enhanced
shutdownAndWait()with forceful shutdown logic specifically for the looper executor (lines 259-307) - Added
RejectedExecutionExceptionhandling to properly notify callbacks when batch submission fails during shutdown (lines 366-381)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java
Outdated
Show resolved
Hide resolved
analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java
Outdated
Show resolved
Hide resolved
analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java
Outdated
Show resolved
Hide resolved
analytics/src/main/java/com/segment/analytics/internal/AnalyticsClient.java
Outdated
Show resolved
Hide resolved
| private void waitForLooperCompletion() { | ||
| if (looperFuture != null) { | ||
| try { | ||
| // Wait for the looper to complete processing the STOP message and finish | ||
| // Use a reasonable timeout to avoid hanging indefinitely | ||
| looperFuture.get(5, TimeUnit.SECONDS); | ||
| log.print(VERBOSE, "Looper completed successfully."); | ||
| } catch (Exception e) { | ||
| log.print(ERROR, e, "Error waiting for looper to complete: %s", e.getMessage()); | ||
| // Cancel the looper if it's taking too long or if there's an error | ||
| if (!looperFuture.isDone()) { | ||
| looperFuture.cancel(true); | ||
| log.print(VERBOSE, "Looper was cancelled due to timeout or error."); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new waitForLooperCompletion() method lacks test coverage. Consider adding tests to verify:
- Successful looper completion within timeout
- Looper timeout and cancellation behavior
- Exception handling during looper wait
- Behavior when
looperFutureis null
This is important for ensuring the shutdown race condition fix works as intended.
| try { | ||
| networkExecutor.submit( | ||
| BatchUploadTask.create(AnalyticsClient.this, batch, maximumRetries)); | ||
| } catch (RejectedExecutionException e) { | ||
| log.print( | ||
| ERROR, | ||
| e, | ||
| "Failed to submit batch %s to network executor during shutdown. Batch will be lost.", | ||
| batch.sequence()); | ||
| // Notify callbacks about the failure | ||
| for (Message msg : batch.batch()) { | ||
| for (Callback callback : callbacks) { | ||
| callback.failure(msg, e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new RejectedExecutionException handling lacks test coverage. Consider adding a test that verifies:
- The error is properly logged when batch submission fails during shutdown
- Callbacks are notified with
failure()for each message in the rejected batch - The looper continues processing after a rejection
This ensures that messages aren't silently lost without callback notification.
This pull request improves the shutdown process and reliability of the
AnalyticsClientby ensuring that background tasks are properly completed or cancelled during shutdown, and by handling potential race conditions and errors more robustly. The main changes focus on ensuring that the message processing looper and network executor are gracefully terminated, and that any failures during shutdown are properly logged and handled.Improvements to shutdown and task management:
Future<?> looperFuturefield to track the background looper task, and updated initialization to store the submitted looper's future. (AnalyticsClient.java)waitForLooperCompletion()method to wait for the looper to finish processing all messages before shutting down executors, preventing race conditions where batches could be lost. (AnalyticsClient.java)shutdownAndWaitmethod to handle forceful shutdowns, log detailed information, and specifically address the looper executor's shutdown process, including handling interrupts and reporting on tasks that fail to terminate. (AnalyticsClient.java)