kdavis-mozilla
commented
8 years ago @Cwiiis @andrenatal I'm inclined to ship with this bug in our code as it doesn't seem to have any adverse effects for our use case. What do you think?
Open kdavis-mozilla opened 8 years ago
kdavis-mozilla
commented
8 years ago @Cwiiis @andrenatal I'm inclined to ship with this bug in our code as it doesn't seem to have any adverse effects for our use case. What do you think?
Cwiiis
commented
8 years ago I don't think it's a show-stopper, as you say.
What is restarting the vaani client though? Those revisions are the same, so git-auto-updater won't kill the process. It prints 'Updating...' and 'Updated.' when it actually updates, so it doesn't appear to be calling update(), which is the only piece of code that kills the command process.
kdavis-mozilla
commented
8 years ago It does restart vaani.client.
The core of it is this code..
...
if (currentRev != latestRev) {
console.log('Updating...');
update();
} else {
if (!commandProcess) {
startCommand();
}
scheduleUpdateCheck();
}
...Where startCommand() is defined by
function startCommand() {
if (conf.command) {
total_boots++;
conf.command.args[1] = total_boots;
console.log('start vaani', conf.command.args);
commandProcess = ChildProcess.spawn(conf.command.name, conf.command.args,
{ stdio: 'inherit' });
commandProcess.on('exit', () => { commandProcess = null; });
}
}where conf.command.name is /opt/vaani.raspberrypi/client/update.sh which looks like this
set -e
cd /opt/vaani.client
export PKG_CONFIG_PATH=/opt/sphinxbase:/opt/pocketsphinx
#npm install
systemctl --system set-environment VAANI_BOOTS=$1
systemctl restart vaaniSo basically this means it calls systemctl restart vaani each time.
Cwiiis
commented
8 years ago Ah, this isn't good... Because the script isn't directly running the client, it isn't being shut down when we do the update, and it's being restarted every time an update is checked for...
Ideally, this script (and I'm not sure off the top of my head how to do this) would wait for SIGINT after starting the service, and stop the service. Instead, it starts the service and immediately exits, so the updater can't do what it's designed to do.
Cwiiis
commented
8 years ago So a quick search reveals an interesting idea (I think) - the script can send itself SIGSTOP, and git-auto-updater could send SIGCONT instead of SIGINT (which it can do by specifying '-s SIGCONT' on the command line). I'll test this out.
kdavis-mozilla
commented
8 years ago Let me know how the test goes
Cwiiis
commented
8 years ago Works. So, in the script, after starting the service, you could put:
kill -SIGSTOP $$ systemctl stop vaani
This will send SIGSTOP to the current process, then after it continues, will stop the vaani service.
Then, when starting git-auto-updater, add the command-line parameter '-s SIGCONT'. This will continue the process, which will then stop the vaani service and quit. After it quits, git-auto-updater will update the repo and restart the script (but only when there's actually a change - otherwise it won't send the signal and the service will continue running).
Cwiiis
commented
8 years ago Let me know if you want pull requests for this (and if you could point me to the repos, if you know them off-hand, that'd save me a minute or two :))
When a scheduled check for an update happens, the check for the update and restart of the managed application can happen so quickly as to occur in the same minute the scheduled check is to occur.
After the restart when this occurs, the schedule for the next check is made and it is found that this next check should occur now, as we're still in the same one minute time window. So the next schedule is made for 0 seconds. This scheduled check for an update happens immediately, and this process repeats until the one minute window is over.
This yields a log of the form...
In the end this really harmless for the vaani.client use case as, other than re-starting the managed application multiple times, it has no side effects.
Furthermore, when an update is really downloaded, I'd guess the wait for the download would take enough time to push the reschedule in to the next minute and this wouldn't be noticed.
However, this is still a bug, though a harmless one, for the vaani.client use case.