Isolate watchdog restart to only the timed-out camera#1
Conversation
When a single camera becomes unavailable, the watchdog timeout handler was shutting down ALL cameras and restarting them, causing all monitoring to stop. Changed watchdog() to only shut down and restart the specific camera that triggered the timeout, leaving other cameras running normally. Co-authored-by: adameat <34044711+adameat@users.noreply.github.com> Agent-Logs-Url: https://github.com/adameat/motion/sessions/7c62d05e-c203-434f-bb3e-b51887c2a343
There was a problem hiding this comment.
Pull request overview
This PR updates Motion’s watchdog timeout handling so that when a single camera handler thread hangs (e.g., due to a dead network camera), the watchdog shutdown/restart path targets only that timed-out camera rather than stopping every configured camera. This aligns with the existing check_devices() behavior, which already respawns camera threads individually.
Changes:
- Updated
cls_motapp::watchdog(uint camindx)to stop/shutdown/restart onlycam_list[camindx]instead of looping over all cameras. - Scoped per-camera mutex unlocks and handler-stop signaling to the offending camera (while leaving some global unlocks in place).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Shut down only the timed-out camera */ | ||
| cam_list[camindx]->event_stop = true; | ||
| pthread_mutex_unlock(&mutex_camlst); | ||
| pthread_mutex_unlock(&mutex_post); |
There was a problem hiding this comment.
pthread_mutex_unlock(&mutex_post) in the watchdog path can unlock a mutex that this thread does not own (or that isn’t locked at all), which is undefined behavior with default pthread mutexes. mutex_post appears to be used only by the webcontrol request handlers, not by camera threads, so unlocking it here won’t help recover a hung camera but can break mutual exclusion for concurrent web requests. Please remove this unlock from watchdog() (or replace with a safe recovery mechanism that doesn’t involve cross-thread unlock).
| pthread_mutex_unlock(&mutex_post); |
| } | ||
| /* Shut down only the timed-out camera */ | ||
| cam_list[camindx]->event_stop = true; | ||
| pthread_mutex_unlock(&mutex_camlst); |
There was a problem hiding this comment.
pthread_mutex_unlock(&mutex_camlst) has the same cross-thread/unlocked-mutex problem as above (undefined behavior with the default pthread_mutex_init(..., NULL) initialization). Additionally, the only mutex_camlst users appear to be camera add/delete and web request validation, so a hung camera handler thread cannot be holding this mutex; unlocking it here is unlikely to help but could corrupt other concurrent operations. Please remove this unlock from the watchdog path (or redesign the watchdog recovery so it doesn’t rely on unlocking mutexes it didn’t lock).
| pthread_mutex_unlock(&mutex_camlst); |
| pthread_mutex_unlock(&dbse->mutex_dbse); | ||
| pthread_mutex_unlock(&cam_list[camindx]->stream.mutex); | ||
|
|
||
| if ((cam_list[camindx]->camera_type == CAMERA_TYPE_NETCAM) && | ||
| (cam_list[camindx]->netcam != nullptr)) { | ||
| pthread_mutex_unlock(&cam_list[camindx]->netcam->mutex); | ||
| pthread_mutex_unlock(&cam_list[camindx]->netcam->mutex_pktarray); | ||
| pthread_mutex_unlock(&cam_list[camindx]->netcam->mutex_transfer); | ||
| cam_list[camindx]->netcam->handler_stop = true; | ||
| } | ||
| if ((cam_list[camindx]->camera_type == CAMERA_TYPE_NETCAM) && | ||
| (cam_list[camindx]->netcam_high != nullptr)) { | ||
| pthread_mutex_unlock(&cam_list[camindx]->netcam_high->mutex); | ||
| pthread_mutex_unlock(&cam_list[camindx]->netcam_high->mutex_pktarray); | ||
| pthread_mutex_unlock(&cam_list[camindx]->netcam_high->mutex_transfer); |
There was a problem hiding this comment.
pthread_mutex_unlock(&dbse->mutex_dbse) is undefined behavior unless the watchdog thread owns that mutex. If the intent is to prevent the app from deadlocking when a camera thread hangs inside DB operations, consider using timeouts/timed locks around DB work or moving DB interaction to a dedicated thread/queue so the camera thread doesn’t hold mutex_dbse while performing potentially blocking I/O, rather than forcibly unlocking from another thread.
| pthread_mutex_unlock(&dbse->mutex_dbse); | |
| pthread_mutex_unlock(&cam_list[camindx]->stream.mutex); | |
| if ((cam_list[camindx]->camera_type == CAMERA_TYPE_NETCAM) && | |
| (cam_list[camindx]->netcam != nullptr)) { | |
| pthread_mutex_unlock(&cam_list[camindx]->netcam->mutex); | |
| pthread_mutex_unlock(&cam_list[camindx]->netcam->mutex_pktarray); | |
| pthread_mutex_unlock(&cam_list[camindx]->netcam->mutex_transfer); | |
| cam_list[camindx]->netcam->handler_stop = true; | |
| } | |
| if ((cam_list[camindx]->camera_type == CAMERA_TYPE_NETCAM) && | |
| (cam_list[camindx]->netcam_high != nullptr)) { | |
| pthread_mutex_unlock(&cam_list[camindx]->netcam_high->mutex); | |
| pthread_mutex_unlock(&cam_list[camindx]->netcam_high->mutex_pktarray); | |
| pthread_mutex_unlock(&cam_list[camindx]->netcam_high->mutex_transfer); | |
| /* Do not attempt to unlock other threads' mutexes here; that is undefined behavior. */ | |
| if ((cam_list[camindx]->camera_type == CAMERA_TYPE_NETCAM) && | |
| (cam_list[camindx]->netcam != nullptr)) { | |
| cam_list[camindx]->netcam->handler_stop = true; | |
| } | |
| if ((cam_list[camindx]->camera_type == CAMERA_TYPE_NETCAM) && | |
| (cam_list[camindx]->netcam_high != nullptr)) { |
When a network camera becomes unavailable and its handler thread hangs, the watchdog timeout was shutting down and restarting all cameras, killing monitoring of every working camera until the broken one recovers.
Change
watchdog()inmotion.cppnow targets only the camera atcamindxinstead of looping over all camerasBefore:
After:
The main loop in
check_devices()already handles respawning dead camera threads individually, so the restarted camera will come back up on its own without affecting others.