Page MenuHomePhabricator

Allow BBPromise cancellation
Closed, ResolvedPublic8 Estimated Story Points

Description

In chromium-render project we need to use the cancellable promises. This can be easily achieved by calling:

BBPromise.config({
  cancellable: true 
});

The problem is that this code has to be executed before any promise is created, otherwise it fails with an error cannot enable cancellation after promises are in use Service-runner uses promises for everything, even reading a config files. Projects have the server.js file, which is not executed by service-runner (only thing it does it's calling the service-runner), so we cannot put that statement there. App.js is already too late as promises were created.

Proposed solution:

  • an env variable: APP_ENABLE_CANCELLABLE_PROMISES, when is set, service-runner will call BBPromise.config() with proper parameter set as a first thing it does (before creating any promise)
  • ??

Related Objects

StatusSubtypeAssignedTask
Resolvedovasileva
ResolvedNone
ResolvedBawolff
Resolvedphuedx
Resolvedmobrovac
Resolvedmobrovac
Resolvedphuedx
ResolvedJdrewniak
Resolvedphuedx
Resolvedphuedx
Resolvedphuedx
Resolvedphuedx
DeclinedNone
Resolvedbmansurov
Resolvedmobrovac
Resolvedovasileva
InvalidNone
ResolvedJdlrobson
Resolvedphuedx
Resolvedphuedx
Resolvedholger.knust
ResolvedTgr
Resolvedjijiki
ResolvedMSantos
Resolvedmobrovac
Resolvedovasileva
Resolvedphuedx
Declinedpmiazga
ResolvedDzahn
Resolvedpmiazga
Duplicateholger.knust
ResolvedMSantos
ResolvedTgr
ResolvedJohan
OpenNone
Resolvedovasileva
InvalidNone
Resolvedmobrovac
Resolvedphuedx
Resolvedpmiazga

Event Timeline

I've proposed the env variable solution, but after sleeping on it I do not think it's a great one, it's more like a workaround. Env variables are supposed to be a configuration and in this case, cancellable promises are a requirement for the software to even work.

We can not make it a part of the config, because the config is read by service-runner in a promise, so it will not work.

We can not make promises cancellable by default, cause AFAIK that will make BB do more work and use more CPU/memory

We can change puppet and pachage.json scripts to run this specific service via some wrapper over service-runner, but I think it's a worse workaround then env var.

Summary: even though the env var solution is ugly, it's the only one feasible I can think of right now.

Jdlrobson set the point value for this task to 8.Nov 8 2018, 10:21 PM
Jdlrobson added a subscriber: Jdlrobson.

via async estimation but we're hoping that most of this is just waiting.

A link to Slack? I don't have it, could you please copy-paste what you meant @Jdlrobson ?

@Pchelolo

Votes for 'https://phabricator.wikimedia.org/T209070'
The votes are in.
@pmiazga: 5, @Stephen: 5, @Jdlrobson: 8, @nray: 3
Stats: median: 5.0, mean: 5.2, min: 3.0, max: 8.0

jdlrobson [23 hours ago]
Proton has a habit of growing. Only one team member actively involved and another team. Proton tasks don't zip through the board and are worst offenders of being in sprint for more than 2weeks. kWhy wouldnt it be anything other than an 8 with that in mind?

Nick Ray [23 hours ago]
yeah, I looked at the change and it looked to be pretty small, but after thinking about it more, it would be a huge context switch if I were to work on it (easily 5 points worth). So I will go up to 8

jdlrobson [20 hours ago]
@Stephen @pmiazga can you go up to an 8?

stephen [20 hours ago]
yeah, i was probably too low on this. 8 sounds great

pmiazga [20 hours ago]
yeah, I can go up to 8, I voted 5 as I see lots of risk but honestly this is not the risk on our side, I came with an idea and I made a patch. Now I think that we have two possibilities, or Services agree with my patch (that is kinda a hack), or they have to come with their own solution. So honestly there is not that much to do on our side, it's just waiting. We did what we could, and I don't expect other work on our side. The ServiceRunner is IMHO too important thing to mess, if Services want the Promise cancellation they have to provide support for that.
I don't know Services internals or what is the ServiceRunner setup on production (I only know we use systemd to call service-runner directly). If there is more work to do, then the Services team should be responsible for that.

^ that. I'm not sure how relevant these slack convos are outside the team but they help us uncover risk and make the team mindful of all the things we are taking on at the same time. I just figured if there's a link understanding the estimate is helpful. Lemme know i appreciate any feedback here even if this is just noise!

mobrovac assigned this task to pmiazga.

Merged and published, resolving.