Iox #324 roudi improve app shutdown

[marthtz]

Introduce 2-step shutdown procedure for apps/processes.
First with a SIGTERM and if they have not terminated after finallKillTime they're killed with SIGKILL.

[orecham]

Can this not lead to races ?

[marthtz]

That's right. Could be dangerous. See @budrus, @elBoberido comments above. Will be addressed.

[elBoberido]

how about passing a reference to a lock guard to this function, then it's impossible to call this without locking a mutex beforehand.
This should maybe be a rule of thumb when we have functions which need to be called with a locked mutex. Let the type system help us, memories are like tears in the rain ;)

[marthtz]

Added a lock guard reference.

[MatthiasKillat]

Can the list not be wrapped in a smart lock to avoid this? In most cases it is best that the data structure itself only provides access protected by mutex. If this is not easily possible (requires much refactoring, or undesired for some technical reason) the approach suggested by @elBoberido is the next best thing I guess (we could unfortunately pass an unreleated mutex in this lock_guard...).

[elfenpiff]

I agree with @MatthiasKillat . I would also explore and try to wrap the smart_lock around this list. Is this possible @marthtz

[elBoberido]

@elfenpiff then this has to be massively refactored or a recursive mutex has to be used. Keep in mind, the mutex might have to wrap more than the list

[elfenpiff]

Okay then I would say leave it ... I didn't see that coming.

[sculpordwarf]

removeProcess is private and only called twice in killAllProcesses. killAllProcesses already takes the lock. Making a mutex recursive, because you can`t manage the calls correctly in your private functions is not the right way in my opinion. If you are afraid of accidentally use your own private function wrong, I would recommend to use a subfunction in killAllProcesses via lambda in c++ and define it after the lock to make it even more clear:

void ProcessManager::killAllProcesses(const units::Duration finalKillTime) noexcept
{
    std::lock_guard<std::mutex> g(m_mutex);
    auto removeProcess = [](ProcessList_t::iterator& processIter) 
    {
       if (processIter != m_processList.end())
       {
          m_portManager.deletePortsOfProcess(processIter->getName());
          m_processIntrospection->removeProcess(processIter->getPid());
          processIter = m_processList.erase(processIter); // delete application
          return true;
       }
       return false;
    };
    cxx::vector<bool, MAX_PROCESS_NUMBER> processStillRunning(m_processList.size(), true);
    int i = 0;
    .
    .
    .
}

[elfenpiff]

I think we reached now the maximum number of arguments for the good old roudi ctor. I would suggest the following:

1. Create a struct called struct RoudiStartupParameters and move all the arguments from here in there.
2. Use this struct RoudiStartupParameters in this ctor instead of all the arguments which provides us with the big advantage that we only have to change one place if we add another argument to roudi.
3. Use it like

RouDi myRoudi(RoudiStartupParameters{memoryInterface, portManager, config::MonitoringMode::ON ....});

then we do not have to change much in the source code. We just have to add RoudiStartupParameters

[marthtz]

What about creating a new issue for that with a separate PR to follow?! That'll help keep to keep our PRs smaller.

[elfenpiff]

In this case I would disagree. The code changes should be minor and the struct would only need some member without any logic at all.

[elBoberido]

we already have something which we could use, it's the RouDiConfig ;)

[marthtz]

Checkout latest commit.

[elfenpiff]

What happens when m_processList is empty. Then it would point to m_processList.end() since it is equal to begin in this case and removeProcess would segfault since we are accessing an invalid element.

[elBoberido]

the method checks if it is iterator.end() so nothing bad happens. But you are right, it's not obvious from the function call

[elBoberido]

how about passing a reference to a lock guard to this function, then it's impossible to call this without locking a mutex beforehand.
This should maybe be a rule of thumb when we have functions which need to be called with a locked mutex. Let the type system help us, memories are like tears in the rain ;)

[elBoberido]

the method checks if it is iterator.end() so nothing bad happens. But you are right, it's not obvious from the function call

[elfenpiff]

resolve bug in ProcessManager::registerProcess

[elBoberido]

at one day, this needs to go to RouDiConfig and also made available in the TOML config

[marthtz]

That's probably also valid for some other predefined values (e.g. discovery interval, etc.), isn't it?

[dkroenke]

I would see this topic in a separate PR/issue. I created it here: #353

[budrus]

@ithier @elfenpiff LGTM?