From 42737cb9e96b889d0fdfd48926778a38566d927b Mon Sep 17 00:00:00 2001 From: Pablo Carranza Velez Date: Wed, 12 Dec 2018 20:56:14 -0300 Subject: [PATCH] Fix a race condition that could cause an unnecessary restart of a service immediately after download Up to now, there was a slim but non-zero chance that an image would be downloaded between the call to `@getTarget` inside deviceState (which gets the target state and creates Service objects using information from available images), and the call to `@images.getAvailable` in ApplicationManager (which is used to determine whether we should keep waiting for a download or start the service). If this race condition happened, then the ApplicationManager would infer that a service was ready to be started (because the image appears as available), but would have incomplete information about the service because the image wasn't available when the Service object was created. The result would be that the service would be started, and then immediately on the next applyTarget the ApplicationManager would try to kill it and restart it to update it with the complete information from the image. This patch changes this behavior by ensuring that all of the additional information about the current state, which includes available images, is gathered *before* building the current and target states that we compare. This means that if the image is downloaded after the call to getAvailable, the Service might be constructed with all the information about the image, but it won't be started until the next pass, because ApplicationManager will treat it as still downloading. Change-type: patch Signed-off-by: Pablo Carranza Velez --- src/application-manager.coffee | 41 ++++++++++++++++++---------------- src/device-state.coffee | 14 +++++++----- 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/src/application-manager.coffee b/src/application-manager.coffee index b30338cd..a04a7fc0 100644 --- a/src/application-manager.coffee +++ b/src/application-manager.coffee @@ -906,26 +906,29 @@ module.exports = class ApplicationManager extends EventEmitter return Promise.reject(new Error("Invalid action #{step.action}")) @actionExecutors[step.action](step, { force, skipLock }) - getRequiredSteps: (currentState, targetState, ignoreImages = false) => + getExtraStateForComparison: => @config.get('localMode').then (localMode) => - Promise.join( - @images.isCleanupNeeded() - @images.getAvailable(localMode) - @images.getDownloadingImageIds() - @networks.supervisorNetworkReady() - @config.get('delta') - (cleanupNeeded, availableImages, downloading, supervisorNetworkReady, delta) => - conf = _.mapValues({ delta, localMode }, (v) -> checkTruthy(v)) - if conf.localMode - cleanupNeeded = false - @_inferNextSteps(cleanupNeeded, availableImages, downloading, supervisorNetworkReady, currentState, targetState, ignoreImages, conf) - .then (nextSteps) => - if ignoreImages and _.some(nextSteps, action: 'fetch') - throw new Error('Cannot fetch images while executing an API action') - @proxyvisor.getRequiredSteps(availableImages, downloading, currentState, targetState, nextSteps) - .then (proxyvisorSteps) -> - return nextSteps.concat(proxyvisorSteps) - ) + Promise.props({ + cleanupNeeded: @images.isCleanupNeeded() + availableImages: @images.getAvailable(localMode) + downloading: @images.getDownloadingImageIds() + supervisorNetworkReady: @networks.supervisorNetworkReady() + delta: @config.get('delta') + localMode + }) + + getRequiredSteps: (currentState, targetState, extraState, ignoreImages = false) => + { cleanupNeeded, availableImages, downloading, supervisorNetworkReady, delta, localMode } = extraState + conf = _.mapValues({ delta, localMode }, (v) -> checkTruthy(v)) + if conf.localMode + cleanupNeeded = false + @_inferNextSteps(cleanupNeeded, availableImages, downloading, supervisorNetworkReady, currentState, targetState, ignoreImages, conf) + .then (nextSteps) => + if ignoreImages and _.some(nextSteps, action: 'fetch') + throw new Error('Cannot fetch images while executing an API action') + @proxyvisor.getRequiredSteps(availableImages, downloading, currentState, targetState, nextSteps) + .then (proxyvisorSteps) -> + return nextSteps.concat(proxyvisorSteps) serviceNameFromId: (serviceId) => @getTargetApps().then (apps) -> diff --git a/src/device-state.coffee b/src/device-state.coffee index 276b6c54..0605e09e 100644 --- a/src/device-state.coffee +++ b/src/device-state.coffee @@ -584,17 +584,19 @@ module.exports = class DeviceState extends EventEmitter @applyBlocker .then => @usingInferStepsLock => - Promise.join( - @getCurrentForComparison() - @getTarget({ initial, intermediate }) - (currentState, targetState) => + @applications.getExtraStateForComparison() + .then (extraState) => + Promise.all([ + @getCurrentForComparison() + @getTarget({ initial, intermediate }) + ]) + .then ([ currentState, targetState ]) => @deviceConfig.getRequiredSteps(currentState, targetState) .then (deviceConfigSteps) => if !_.isEmpty(deviceConfigSteps) return deviceConfigSteps else - @applications.getRequiredSteps(currentState, targetState, intermediate) - ) + @applications.getRequiredSteps(currentState, targetState, extraState, intermediate) .then (steps) => if _.isEmpty(steps) @emitAsync('apply-target-state-end', null)