Workaround for Windows file locking issues during Message Processor task deletion - #4571
Workaround for Windows file locking issues during Message Processor task deletion#4571ochnios wants to merge 1 commit into
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| private boolean deleteWithRetry(File file) { | ||
| if (!file.exists()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| private boolean deleteWithRetry(File file) { | |
| if (!file.exists()) { | |
| return false; | |
| } | |
| private boolean deleteWithRetry(File file) { | |
| if (!file.exists()) { | |
| return false; | |
| } | |
| log.info("Attempting to delete file: " + file.getAbsolutePath()); |
| for (int attempt = 1; attempt <= TASK_DELETE_MAX_ATTEMPTS; attempt++) { | ||
| if (file.delete()) { | ||
| log.debug("File deleted successfully on attempt " + attempt + ": " + file.getAbsolutePath()); | ||
| return true; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| for (int attempt = 1; attempt <= TASK_DELETE_MAX_ATTEMPTS; attempt++) { | |
| if (file.delete()) { | |
| log.debug("File deleted successfully on attempt " + attempt + ": " + file.getAbsolutePath()); | |
| return true; | |
| for (int attempt = 1; attempt <= TASK_DELETE_MAX_ATTEMPTS; attempt++) { | |
| if (file.delete()) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("File deleted successfully on attempt " + attempt + ": " + file.getAbsolutePath()); | |
| } | |
| return true; |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
WalkthroughThe change introduces retry-based deletion logic to FileBasedTaskRepository, replacing direct file deletion calls with a private deleteWithRetry method that attempts up to 3 times with 50ms delays between attempts. This addresses transient file locking issues that prevent task file deletion during redeployment operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
ochnios
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/mediation/tasks/org.wso2.micro.integrator.ntask.core/src/main/java/org/wso2/micro/integrator/ntask/core/impl/FileBasedTaskRepository.java (1)
376-379: Restore the interrupt status after catchingInterruptedException.When catching
InterruptedException, best practice is to restore the interrupt flag so that code higher in the call stack can detect that an interrupt occurred.Suggested fix
} catch (InterruptedException e) { + Thread.currentThread().interrupt(); log.warn("Interrupted while waiting to retry file deletion: " + file.getAbsolutePath()); break; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/mediation/tasks/org.wso2.micro.integrator.ntask.core/src/main/java/org/wso2/micro/integrator/ntask/core/impl/FileBasedTaskRepository.java
🔇 Additional comments (4)
components/mediation/tasks/org.wso2.micro.integrator.ntask.core/src/main/java/org/wso2/micro/integrator/ntask/core/impl/FileBasedTaskRepository.java (4)
64-65: Constants are reasonable for the retry mechanism.The values (3 attempts, 50ms delay) provide a sensible balance between resilience and responsiveness, with a maximum additional blocking time of ~150ms. Given this is a workaround for a Windows-specific edge case, hardcoded values are acceptable.
274-276: LGTM!Clean delegation to the retry helper with correct return value handling.
281-284: LGTM!Correctly gates the in-memory map cleanup on successful file deletion, preventing state inconsistency.
359-385: Pragmatic workaround for Windows file locking.The
System.gc()hint is a known last-resort approach for releasing file handles held by phantom references on Windows. While non-deterministic (JVM may ignore the hint), the retry loop with delays provides reasonable resilience. Given the PR description notes this matches patterns elsewhere in the codebase and was the only consistently working solution in testing, this is acceptable.
|
@rosensilva could you take a look at this |
1 participant
Purpose
Resolves #4570
Analysis
I couldn't find any place in code with possible resource leak like unclosed stream or something like that so I assume the issue lies somewhere deeper (some bug on JVM for Windows like this?). After failed deletion I can still see open file handles in Windows Resource Monitor (
resmon.exe). Both are from the same Micro Integrator process:A similar workaround exists elsewhere in the codebase to handle "known bug in windows" (here). This PR applies a similar strategy.
Goals
Approach
I introduced a
deleteWithRetrymethod inFileBasedTaskRepository. Inside the retry loop,System.gc()is invoked when deletion of the registry files fails. Triggering GC forces the release of file handles which are kept, allowing the OS to unlock and delete the file. A small delay is added between retries to allow the OS file system to catch up.I acknowledge that using
System.gc()is a "last resort" workaround. However, given the aggressive file locking this was the only solution that consistently resolved the issue in my testing.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.