Page MenuHomePhabricator

Why does Proton specifically require APP_ENABLE_CANCELLABLE_PROMISES in production but not elsewhere?
Closed, ResolvedPublic

Description

In this change I removed the usage of the APP_ENABLE_CANCELLABLE_PROMISES environment variable from the chromium-render repository, and instead ensured that npm start started service-runner via the server.js stub, which directly sets Bluebird promises to be cancellable:

#!/usr/bin/env node

'use strict';

// Service entry point. Try node server --help for commandline options.

const Promise = require('bluebird');

// enable promise cancellation feature.
Promise.config({ cancellation: true });

// Start the service by running service-runner, which in turn loads the config
// (config.yaml by default, specify other path with -c). It requires the
// module(s) specified in the config 'services' section (app.js in this
// example).
const ServiceRunner = require('service-runner');
new ServiceRunner().start();

This works well locally and on the Beta Cluster.

However, removing the APP_ENABLE_CANCELLABLE_PROMISES setting from the service's Helm chart and deploying resulted in promise cancellation not being enabled and requests failing. This caused a ~20 min PDF rendering outage from approximately 14:52-15:14 UTC on 2020-07-28 while I noticed the error, replaced the environment variable in the Helm chart, and redeployed.

I don't know why we specifically need the environment variable in production, because based on my reading of the Helm chart, the service start command seems to use the same server.js entrypoint as we use locally and in Beta, in which promise cancellation is enabled directly.

Event Timeline

Not sure why exactly yet, but the env variable APP_ENABLE_CANCELLABLE_PROMISES is needed when you set num_workers in the config.yaml file with any number other than 0, that's why production have a different behavior. I think we should drop the line Promise.config({ cancellation: true }); and always require the environment variable.

[...] I think we should drop the line Promise.config({ cancellation: true }); and always require the environment variable.

Nevermind, this is still required. I suggest this ticket is marked as invalid.

Mholloway reassigned this task from Mholloway to MSantos.

the env variable APP_ENABLE_CANCELLABLE_PROMISES is needed when you set num_workers in the config.yaml file with any number other than 0, that's why production have a different behavior.

Oh, nice catch! Yep, it makes sense that the forked worker processes when running multiple workers also need to know that promises should be cancellable. I don't feel strongly either way about putting back the env variable in the code repo vs. just letting it be.