Uh oh!
There was an error while loading. Please reload this page.
New methods to access processes. - #25
Conversation
jmachowinski
commented
Feb 9, 2021
Fixed alive() method. |
maltewi
left a comment
There was a problem hiding this comment.
Thanks for the effort overworking this part... it's appreciated. But there are a few things to clarify before merging
| throw std::runtime_error(std::string("waitpid failed: ") + strerror(errno)); | ||
| else | ||
| throw std::runtime_error("waitpid returned undocumented value"); | ||
| } |
There was a problem hiding this comment.
I think this function has a bug: the name of the function suggests that it's okay to call it on a non-running process with the expected return value false. This behavior was ensured with the previously present isRunning flag.
Whats the behavior now, if we call a process, kill it and then call alive after the collection of the termination signal again. I think the function would not show the expected behavior.
See also comment regarding killAll and handles.
There was a problem hiding this comment.
This should not be an issue. When the process just finished, waitpid will return the pid of the exited process, hence alive will just return false. When waitpid returns -1, it means that it could not check the status of the process.
| } | ||
| } | ||
| bool Spawner::ProcessHandle::end() |
There was a problem hiding this comment.
What's the reason for having this function as well as killDeployment + wait(). Looks like is a re-implementaion of the same thing (also re-implementing the sendSig* functions), and it looks like end() is called nowhere.. so I wonder what's its purpose.
There was a problem hiding this comment.
Completely true, I forgot to remove this one. It is now gone.
| } | ||
| } | ||
| bool Spawner::killDeployment(const std::string &dplName) |
There was a problem hiding this comment.
look at comment above regarding end()
| Spawner::ProcessHandle &handle = getDeployment(dplName); | ||
| handle.sendSigInt(); | ||
| if(!handle.wait()) |
There was a problem hiding this comment.
the parameters for wait should be configurable here. I'd suggest to add them also to killDeployment
| ProcessHandle *handle = new ProcessHandle(deployment, redirectOutput, logDir); | ||
| handles.push_back(handle); |
There was a problem hiding this comment.
I think handles never get removed from the handles-variable. Thus there's no real cleanup happening. This problem apparently was present also before the MR, but with the issue described at alive this might lead to unexpected behavior...
e.g. A single deployment was killed, afterwards killAll is called. In this case alive will be called on the already-terminated process again.
There was a problem hiding this comment.
This seems to be the case, I need to look into this a bit further. But I think this issue existed before already.
skasperski
commented
Feb 9, 2021
Oh, that message was kinda deprecated. The actual functional error was alrady solved before. However I removed the std::cout calls in favor of exceptions, thus enabling the caller to see that something actually went wrong here. As the name of the method suggests, we don't want to have a full status update but just an info whether the process is still running or not. |
No description provided.