fix: process_video silent hang on callback error
#2022
Conversation
realh4m
changed the title
|
|
|
@realh4m, thank you for your contribution and suggested fix. Could you also include a test to verify that it resolves the issue? 🦩 |
Borda
changed the title
process_video silent hang on callback error
There was a problem hiding this comment.
Pull request overview
This PR fixes a silent hang issue in process_video when the callback function raises an exception during video processing. The fix ensures that exceptions are caught, the pipeline is properly shut down, and the error is re-raised to the caller.
Changes:
- Added exception tracking (
exception_in_worker) to capture and re-raise callback errors - Introduced
read_finishedflag to conditionally drain the read queue in the finally block - Modified the finally block to drain remaining frames from the read queue when an exception occurs, preventing deadlock
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as exc: | ||
| exception_in_worker = exc | ||
| break | ||
|
|
||
| frame_write_queue.put(processed_frame) | ||
| progress_bar.update(1) |
There was a problem hiding this comment.
When an exception occurs in the callback (line 318-320), the code breaks out of the loop without putting the processed frame into frame_write_queue. However, the frame_write_queue.put(processed_frame) statement at line 322 is unreachable after the break statement at line 320. This is correct behavior, but there's a potential issue: if the exception occurs, no frame is written to the write queue for that iteration, but the progress bar was already updated. The progress bar should only be updated after successful processing and queuing. Consider moving progress_bar.update(1) inside the try block after line 322 to ensure it only updates on success.
| except Exception as exc: | |
| exception_in_worker = exc | |
| break | |
| frame_write_queue.put(processed_frame) | |
| progress_bar.update(1) | |
| frame_write_queue.put(processed_frame) | |
| progress_bar.update(1) | |
| except Exception as exc: | |
| exception_in_worker = exc | |
| break |
| if not read_finished: | ||
| while True: | ||
| read_item = frame_read_queue.get() | ||
| if read_item is None: | ||
| break |
There was a problem hiding this comment.
The drain loop uses a blocking get() which could potentially hang if the reader thread has crashed or is otherwise unable to send the terminating None sentinel. Consider using get(timeout=...) with an appropriate timeout to prevent indefinite blocking in edge cases where the reader thread fails unexpectedly.
| except Exception as exc: | ||
| exception_in_worker = exc | ||
| break | ||
|
|
||
| frame_write_queue.put(processed_frame) | ||
| progress_bar.update(1) |
There was a problem hiding this comment.
The variable processed_frame is only assigned inside the try block. If an exception occurs before it's assigned (which is the purpose of this exception handling), the code at line 322 would reference an undefined variable, causing a NameError. However, since line 320 breaks out of the loop when an exception is caught, line 322 is never reached in the exception case. This code structure is confusing and fragile. Consider moving line 322 inside the try block, right after line 317, to make the control flow clearer and ensure the frame is only queued if processing succeeds.
| except Exception as exc: | |
| exception_in_worker = exc | |
| break | |
| frame_write_queue.put(processed_frame) | |
| progress_bar.update(1) | |
| frame_write_queue.put(processed_frame) | |
| progress_bar.update(1) | |
| except Exception as exc: | |
| exception_in_worker = exc | |
| break |
realh4m
force-pushed
the
fix/video-worker-error-propagation
branch
from
0406081 to
fce63f8
Compare
|
Thank you for the feedback. I've added unit tests in |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2022 +/- ##
=========================================
Coverage ? 53%
=========================================
Files ? 61
Lines ? 7090
Branches ? 0
=========================================
Hits ? 3746
Misses ? 3344
Partials ? 0 🚀 New features to boost your workflow:
|
3 participants
Description
Added exception tracking in
process_videoso callback errors raised in the worker loop are surfaced after reader/writer threads finish, preventing silent hangs when the callback crashes mid-stream.Introduced
read_finishedto avoid draining the read queue unnecessarily once EOF is reached.process_videocould hang without any error when the callback raised an exception.Type of change
How has this change been tested, please provide a testcase or example of how you tested the change?
Verified locally with a callback that raises an exception.
Any specific deployment considerations
No additional considerations; behavior change only affects
process_videoerror handling.Docs