Conversation
Co-authored-by: 0x484558 <hex@aleph0.ai>
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues in Windows service lifecycle and error handling remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request improves Windows service lifecycle handling by reducing idle CPU usage, supporting stop requests during pending states, and improving error reporting.
Changes:
- Replaces busy polling with timed stop-event waits.
- Initializes stop coordination earlier.
- Propagates worker and Win32 errors through service status reporting.
Open review items remain in src/main_windows.cpp: handle OVMS_EX_USAGE, initialize the service type before reporting event-creation failures, and recheck the stop flag before publishing RUNNING.
File summaries
| File | Summary |
|---|---|
src/main_windows.hpp |
Adds atomic lifecycle state and error-reporting declarations. |
src/main_windows.cpp |
Implements event-based waiting, stop handling, and service error propagation; three moderate issues remain. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| DEBUG_LOG("serviceWorkerThread: WaitForSingleObject returned error."); | ||
| serviceReportEvent("WaitForSingleObject", waitError); | ||
| serviceWorkerWin32Error.store(waitError); | ||
| ovmsService->error = static_cast<int>(waitError); |
There was a problem hiding this comment.
Remove this, it serves absolutely no semantic purpose but the presence of this line deliberately introduces a danger of data race because another thread owns ovmsService->error there is absolutely no reason to retain a known data race danger.
|
|
||
| void OvmsWindowsServiceManager::setServiceStopStatusPending() { | ||
| serviceStatus.dwControlsAccepted = 0; | ||
| serviceStatus.dwControlsAccepted = SERVICE_ACCEPT_STOP; |
There was a problem hiding this comment.
Stop is already in progress here, this is not necessary. This isn't harmful but I just find the misunderstanding of windows service lifecycle amusing.
| if (serviceStopRequested.load()) { | ||
| break; | ||
| } | ||
| // Tell the service controller we are started | ||
| OvmsWindowsServiceManager::setServiceRunningStatus(); |
There was a problem hiding this comment.
despite the check being immediately before the publication, it is still a check-then-act sequence across threads.
🛠 Summary
JIRA CVS-195009
Windows service lifecycle fixes: remove idle spin, fix the startup stop-event race, preserve/report real stop errors, and allow stop during START_PENDING/STOP_PENDING.. Check the WaitForSingleObject return code and report failure if any.
Based on #4568
🧪 Checklist
``