Conversation
Previously, if alive supervision construction failed inside ProcessInfoNode's constructor, the failure was logged and startup continued anyway with a null supervision handle. Convert ProcessInfoNode's constructor to a Create() factory that returns an error (kErrorBeforeReady) if alive supervision setup fails. Propagate that failure through Graph::Create() (also converted from a plain constructor to a factory, since building the dependency graph can now fail) up to ProcessGroupManager::initializeProcessGroups(), which already fails startup on false. Also fixes a latent bug found while adding this: ProcessInfoNode's move constructor did not preserve start_tries_ or termination_result_, silently resetting configured restart attempts to 1 on every move. This was previously undetectable because the move constructor was never exercised on ordinary construction; Create() now moves through it at least once.
GitHub Actions
Waiting
GitHub Actions
Waiting
|
Hello @WilliamRoebuck I applied the needed changes and ready for review whenever you have the time> |
WilliamRoebuck
left a comment
WilliamRoebuck
left a comment
There was a problem hiding this comment.
Apologies for the delayed response, I have been away.
Unfortunately, I think we'll have to find a different way to escalate the failure, as using a Create method as I proposed means we would not be able to emplace ProcessInfoNodes into the dependency graph, and would need to use a move. I'm sorry to have overlooked this when creating the issue. An alternative solution could be for the caller (Graph) to construct each node's alive supervision and then pass the handle to the ProcessInfoNode's in-place constructor if that construction succeeds. I'll update #606
|
|
||
| const auto index = graph.try_emplace( | ||
| IdentifierHash{name}, std::in_place_type<ProcessInfoNode>, std::move(component_config), process_handling); | ||
| auto node_res = ProcessInfoNode::Create(std::move(component_config), process_handling); |
There was a problem hiding this comment.
I think losing the in-place construction here is a problem for efficiency on large graphs. However, I can see that with the Create method it will be very difficult to emplace. I'll have to change the proposed solution in the issue, I apologise for misleading you
2 participants
Previously, if alive supervision construction failed inside ProcessInfoNode's constructor, the failure was logged and startup continued anyway with a null supervision handle.
Convert ProcessInfoNode's constructor to a Create() factory that returns an error (kErrorBeforeReady) if alive supervision setup fails. Propagate that failure through Graph::Create() (also converted from a plain constructor to a factory, since building the dependency graph can now fail) up to ProcessGroupManager::initializeProcessGroups(), which already fails startup on false.
Also fixes a latent bug found while adding this: ProcessInfoNode's move constructor did not preserve start_tries_ or termination_result_, silently resetting configured restart attempts to 1 on every move. This was previously undetectable because the move constructor was never exercised on ordinary construction; Create() now moves through it at least once.
Fixes #606