Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #121 by making the process disposal logic more robust and idempotent, preventing errors when attempting to shut down a process multiple times or when it's not currently running.
Changes:
- Added an
isDestroyedatomic boolean flag to track process lifecycle state - Implemented idempotent
destroy()method using compareAndSet pattern - Added
isAlive()method to check process status before destruction - Added comprehensive test coverage for multiple disposal scenarios
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/kotlin/com/dprint/services/editorservice/process/EditorProcess.kt | Added atomic boolean flag and isAlive() method to track process state; made destroy() idempotent |
| src/main/kotlin/com/dprint/services/editorservice/v5/EditorServiceV5.kt | Added check to only destroy process if it's alive |
| src/main/kotlin/com/dprint/services/editorservice/v4/EditorServiceV4.kt | Added check to only destroy process if it's alive |
| src/test/kotlin/com/dprint/services/editorservice/v5/EditorServiceV5Test.kt | Added three test cases covering destruction scenarios |
| src/test/kotlin/com/dprint/services/editorservice/process/EditorProcessTest.kt | Added tests for isAlive() and idempotent destroy() behavior |
| gradle.properties | Bumped version to 0.9.1.beta |
| CHANGELOG.md | Added entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process = null | ||
| } | ||
|
|
||
| fun isAlive(): Boolean = !isDestroyed.get() || process != null |
There was a problem hiding this comment.
The logic in the isAlive() method is incorrect. The OR operator should be AND. Currently, the method returns true when either the isDestroyed flag is false OR process is not null. This means that even after destruction (when isDestroyed is true), if process is still not null, it would return true, which is incorrect.
The correct implementation should be: fun isAlive(): Boolean = !isDestroyed.get() && process != null
This ensures that the process is considered alive only when BOTH conditions are met: the isDestroyed flag is false AND the process reference is not null.
| fun isAlive(): Boolean = !isDestroyed.get() || process != null | |
| fun isAlive(): Boolean = !isDestroyed.get() && process != null |
| override fun initialiseEditorService() { | ||
| // If not enabled we don't start the editor service | ||
| if (!project.service<ProjectConfiguration>().state.enabled) return | ||
| // Reset the destroyed flag when reinitializing |
There was a problem hiding this comment.
This comment mentions "Reset the destroyed flag when reinitializing" but doesn't correspond to any actual code in this method. The destroyed flag resetting happens in the initialize() method of EditorProcess, not here. This comment is misleading and should be removed or clarified.
| // Reset the destroyed flag when reinitializing | |
| // Reinitialize the editor process (initialize() resets its internal destroyed flag) |
It is possible that we try and shutdown the process multiple times or when it isn't currently running. This PR should fix the issue and make it more robust.
Fixes #121