fix(process): forcefully disable multiprocessing if the user's machine is bad - #273
Merged
Conversation
Reviewer's GuideAdds a runtime safeguard around all multiprocessing-based operations, automatically switching start methods or falling back to synchronous processing and a global flag when the environment cannot safely use multiprocessing, and updates drive watching and preview pipelines to respect this flag. Sequence diagram for multiprocessing failure and sync fallback in image previewsequenceDiagram
participant User
participant PreviewContainer
participant Application
participant PreviewUtils
participant Utils as multiprocessing_process_error_checker
User->>PreviewContainer: show_image_preview()
PreviewContainer->>Application: read MULTIPROCESSING_PROCESS_ALLOWED
alt multiprocessing allowed
PreviewContainer->>PreviewUtils: resample_file(file_path)
PreviewUtils-->>PreviewContainer: ValueError fds_to_keep
PreviewContainer->>Utils: multiprocessing_process_error_checker(app, exc)
alt checker returns true
Utils-->>PreviewContainer: true
alt start method adjusted
PreviewContainer->>PreviewUtils: resample_file(file_path)
PreviewUtils-->>PreviewContainer: pil_object
else multiprocessing disabled
PreviewContainer->>Application: set MULTIPROCESSING_PROCESS_ALLOWED = False
PreviewContainer->>PreviewUtils: resample_file_sync(file_path)
PreviewUtils-->>PreviewContainer: pil_object
end
else checker returns false
Utils-->>PreviewContainer: false
PreviewContainer-->>User: propagate ValueError
end
else multiprocessing not allowed
PreviewContainer->>PreviewUtils: resample_file_sync(file_path)
PreviewUtils-->>PreviewContainer: pil_object
end
PreviewContainer-->>User: display image preview
Updated class diagram for multiprocessing safeguardsclassDiagram
class Application {
bool MULTIPROCESSING_PROCESS_ALLOWED
watch_for_changes_and_update()
}
class PreviewContainer {
show_resvg_preview()
show_image_preview()
load_pdf_pages(first_page int, last_page int) list~PILImage~
}
class PinnedSidebar {
list~str~ DRIVES
refresh_drives(os_type str) void
}
class PreviewUtils {
+load_svg(file_path str) bytes
+load_svg_sync(file_path str) bytes
+resample_file_sync(file_path str) Image
+resample_sync(image Image) Image
+resample_batch_sync(images list~PILImage~) list~PILImage~
}
class Utils {
+multiprocessing_process_error_checker(app App, exc Exception) bool
}
class DriveUtils {
+get_mounted_drives(os_type str) list~str~
+get_mounted_drives_worker(result_queue multiprocessing.Queue, os_type str) void
}
class DriveWorkers {
+get_mounted_drives(os_type str) list~str~
+get_mounted_drives_worker(result_queue multiprocessing.Queue, os_type str) void
}
Application "1" o-- "1" PinnedSidebar : owns
Application "1" o-- "many" PreviewContainer : uses
PreviewContainer --> Application : app
PinnedSidebar --> Application : app
PreviewContainer ..> PreviewUtils : uses
PreviewContainer ..> Utils : uses
PinnedSidebar ..> DriveUtils : uses
PinnedSidebar ..> Utils : uses
Application ..> DriveWorkers : uses
Application ..> Utils : uses
Flow diagram for multiprocessing_process_error_checker behaviorflowchart TD
A[Exception in multiprocessing operation] --> B{exc is ValueError and message contains fds_to_keep}
B -->|no| Z[Return false]
B -->|yes| C[Get current multiprocessing start method]
C --> D{start method}
D -->|None| E[Try set_start_method forkserver]
E --> F{ValueError cannot find context}
F -->|no error| G[Notify app multiprocessing is now using forkserver]
F -->|error with cannot find context| H[set_start_method spawn]
H --> I[Notify app multiprocessing is now using spawn]
D -->|fork| J[set_start_method forkserver]
J --> G
D -->|forkserver| H
D -->|spawn| K[Set app.MULTIPROCESSING_PROCESS_ALLOWED to False]
G --> Y[Return true]
I --> Y
K --> Y
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
watch_for_changes_and_update,new_drivescan be referenced before assignment (e.g., when the multiprocessing branch times out and doesn't populate the queue), so consider guarding the comparison or ensuringnew_drivesis always set in all control paths where it's later used. - In
multiprocessing_process_error_checker,multiprocessing.set_start_methodcan raiseRuntimeErrorif the start method is already set, and you're not passingforce=True; it would be safer to handle this case explicitly or avoid repeated changes to the start method once initialized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `watch_for_changes_and_update`, `new_drives` can be referenced before assignment (e.g., when the multiprocessing branch times out and doesn't populate the queue), so consider guarding the comparison or ensuring `new_drives` is always set in all control paths where it's later used.
- In `multiprocessing_process_error_checker`, `multiprocessing.set_start_method` can raise `RuntimeError` if the start method is already set, and you're not passing `force=True`; it would be safer to handle this case explicitly or avoid repeated changes to the start method once initialized.
## Individual Comments
### Comment 1
<location path="src/rovr/app.py" line_range="662-671" />
<code_context>
+ if self.MULTIPROCESSING_PROCESS_ALLOWED:
</code_context>
<issue_to_address>
**issue (bug_risk):** Possible use of `new_drives` before assignment when the subprocess finishes but produces no output.
Because `new_drives` is only set inside `elif not result_queue.empty()`, if the subprocess exits within the timeout but the queue is empty, execution reaches `if new_drives != drives():` and raises `UnboundLocalError`. It also makes an empty-but-valid result indistinguishable from an error.
You can avoid this by initializing `new_drives` and checking it explicitly, e.g.:
```python
new_drives: list[str] | None = None
...
if self.MULTIPROCESSING_PROCESS_ALLOWED:
...
elif not result_queue.empty():
new_drives = result_queue.get_nowait()
else:
new_drives = drive_workers.get_mounted_drives(os_type)
if new_drives is not None and new_drives != drives():
self.query_one(PinnedSidebar).reload_pins()
```
</issue_to_address>
### Comment 2
<location path="src/rovr/functions/utils.py" line_range="211-220" />
<code_context>
+def multiprocessing_process_error_checker(app: App, exc: Exception) -> bool:
</code_context>
<issue_to_address>
**issue (bug_risk):** `multiprocessing.set_start_method` calls can raise `RuntimeError` when the start method is already set, which isn’t handled here.
The helper currently only anticipates `ValueError`, but `set_start_method` can also raise `RuntimeError` once the multiprocessing context is initialized (e.g., if this runs more than once per process), turning the intended recovery path into an uncaught exception.
To make this more robust, either:
- Use `force=True` when setting the start method (if acceptable), and/or
- Catch `RuntimeError` and handle it as a non-recoverable configuration (e.g., log and disable `MULTIPROCESSING_PROCESS_ALLOWED`).
For instance:
```python
try:
multiprocessing.set_start_method("forkserver", force=True)
except (ValueError, RuntimeError):
...
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
1 participant
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.
keep retrying the process until a method works, or you just disable multiprocessing (which isnt the best outcome, but whatever tbh i hate how elusive this bug is)
resolves #266 again
Summary by Sourcery
Adjust multiprocessing usage to dynamically fall back to synchronous processing or disable it on problematic systems, improving robustness of drive watching and preview generation.
Bug Fixes:
Enhancements:
Chores: