Fix InterruptedException handling and silently swallowed errors across HDDS and Ozone modules#1
Open
devin-ai-integration[bot] wants to merge 2 commits into
Open
Conversation
…rrors Restore Thread.currentThread().interrupt() in catch blocks that swallow InterruptedException without preserving the interrupt status. Replace an empty catch block in DefaultCAServer with debug logging. Files changed: - RatisHelper: split combined catch to restore interrupt separately - ReconfigurableBase: restore interrupt on shutdown join - LeakDetector: restore interrupt before exiting loop - ContainerCommandResponseBuilders: restore interrupt before re-throwing - RocksDBCheckpointDiffer: restore interrupt before re-throwing (2 sites) - SCMSafeModeManager: restore interrupt in daemon thread - OnDemandContainerScanner: restore interrupt on scan interruption - AbstractBackgroundContainerScanner: restore interrupt on scan interruption - StreamBlockInputStream: restore interrupt in checkError and setCompleted - GrpcOzoneManagerServer: restore interrupt during shutdown - OzoneManager: restore interrupt in EDEKCacheLoader (2 sites) - OzoneManagerDoubleBuffer: restore interrupt during stopDaemon join - AuthorizerLockImpl: restore interrupt before wrapping in OMException (2 sites) - DeleteVolumeHandler: restore interrupt on recursive delete failure - SchedulerService: restore interrupt during shutdown - DefaultCAServer: log swallowed IOException at debug level Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR fixes error handling deficiencies across 16 production source files where
InterruptedExceptionis caught without restoring the thread's interrupt status viaThread.currentThread().interrupt(). Per Java concurrency best practices, swallowing the interrupt flag prevents callers higher in the stack from detecting that an interrupt was requested, which can cause threads to hang indefinitely during shutdown or miss cancellation signals.Additionally, one empty catch block in
DefaultCAServer.checkIfKeysExist()that silently discards anIOExceptionis replaced with debug-level logging.Changes by category
Interrupt status restoration (15 sites across 15 files):
Files fixed:
RatisHelper.attemptUntilTrue— split combinedInterruptedException | IllegalStateExceptioncatch to restore interrupt separatelyReconfigurableBase.shutdownReconfigurationTask— restore interrupt onjoin()LeakDetector.run— restore interrupt before exiting detector loopContainerCommandResponseBuilders.getEchoResponse— restore interrupt before re-throwing asRuntimeExceptionRocksDBCheckpointDiffer— two sites:pruneOlderCompactionLogsandpruneNonLeafSstFiles, restore interrupt before re-throwingSCMSafeModeManager.startRefresh— restore interrupt in daemon refresh threadOnDemandContainerScanner.scanContainer— restore interrupt on scan interruptionAbstractBackgroundContainerScanner.scanContainers— restore interrupt before setting stopping flagStreamBlockInputStream.StreamingReadResponse— two sites:checkError()andsetCompleted()GrpcOzoneManagerServer.stop— restore interrupt during gRPC shutdownOzoneManager.EDEKCacheLoader.run— two sites: initial delay and retry sleepOzoneManagerDoubleBuffer.stopDaemon— restore interrupt during daemon joinAuthorizerLockImpl— two sites:tryOptimisticReadThrowOnTimeoutandtryWriteLockThrowOnTimeoutDeleteVolumeHandler.execute— restore interrupt on recursive delete failureSchedulerService.destroy— restore interrupt during scheduler shutdownSilent error swallowing (1 site):
DefaultCAServer.checkIfKeysExist— emptycatch (IOException ignored) { }replaced withLOG.debug(...)to aid debugging key-loading failuresWhat is the link to the Apache JIRA
N/A — this is a fork-local improvement.
How was this patch tested?
Each change is a one-line addition of
Thread.currentThread().interrupt()(or a trivial logging addition) with no behavioral change to the surrounding control flow. The changes follow the existing interrupt handling pattern already used correctly elsewhere in the codebase (e.g.,BackgroundService.shutdown,SnapshotDiffManager.closeExecutorService,ContainerBalancerTask). Verified via compilation.Generated-by: Devin (Claude)
Link to Devin session: https://app.devin.ai/sessions/a43e4a35d06b4c6e9a3c88a0638d9288
Requested by: @marcuslin123