Discuss
  • non-self referencial cyclical promises?

non-self referencial cyclical promises?

# Bradley Meck (9 years ago)

I was doing some recursive data structure work and ended up with a cyclical promise that did not use a direct self reference. It can be reduced down to:

var af, a = new Promise(f=>af=f);

var bf, b = new Promise(f=>bf=f);

af(b);bf(a); // the problem

a.then(_=>_) // some env/libs need this to start checking status

According to tc39.github.io/ecma262/#sec-promise-resolve-functions it looks like this should cause a recursive and infinite set of EnqueueJob("PromiseJobs",...)

This code currently does strange things in stable browsers, but doesn't seem to always cause infinite loops in the nightly builds of some things. I was wondering what the expected behavior is here because I am having trouble making sense of things.

# Kris Kowal (9 years ago)

With Mark’s direction, the v2 branch of Q handles "vicious cycles" through the WeakMap that maps promises to their underlying handler. Whenever a promise is resolved with another promise, it walks forward through the chain of promises that the promise handler "became" through resolution. The first promise’s initial handler is a "vicious cycle" rejected promise handler, so if the walk forward through the resolution returns to itself, it remains resolved to the vicious cycle.

kriskowal/q/blob/v2/q.js#L181

# Bergi (9 years ago)

Bradley Meck wrote:

I was doing some recursive data structure work and ended up with a cyclical promise that did not use a direct self reference. It can be reduced down to:

var af, a = new Promise(f=>af=f);
var bf, b = new Promise(f=>bf=f);

af(b);bf(a); // the problem

a.then(_=>_) // some env/libs need this to start checking status

According to tc39.github.io/ecma262/#sec-promise-resolve-functions it looks like this should cause a recursive and infinite set of EnqueueJob("PromiseJobs",...)

I fear that's what the standard says, yes. The ES6 spec does too many (and in some cases, unreasonably many) then calls on promises anyway to be followed by an efficient promise implementation.

Promises/A+ in contrast says

| Implementations are encouraged, but not required, to detect such | recursion and reject promise with an informative TypeError as the | reason.

, Bergi

# Mark S. Miller (9 years ago)

On Wed, Feb 24, 2016 at 11:54 AM, Bergi <a.d.bergi at web.de> wrote:

Bradley Meck wrote:

I was doing some recursive data structure work and ended up with a cyclical promise that did not use a direct self reference. It can be reduced down to:

var af, a = new Promise(f=>af=f);
var bf, b = new Promise(f=>bf=f);

af(b);bf(a); // the problem

a.then(_=>_) // some env/libs need this to start checking status

According to tc39.github.io/ecma262/#sec-promise-resolve-functions it looks like this should cause a recursive and infinite set of EnqueueJob("PromiseJobs",...)

I fear that's what the standard says, yes. The ES6 spec does too many (and in some cases, unreasonably many) then calls on promises anyway to be followed by an efficient promise implementation.

Promises/A+ in contrast says

| Implementations are encouraged, but not required, to detect such | recursion and reject promise with an informative TypeError as the | reason.

I think the standard should require a deterministic error. E < kpreid/e-on-java/blob/master/src/jsrc/org/erights/e/elib/ref/ViciousCycleException.java>,

Q, and my own Q-like system < tvcutsem/es-lab/blob/master/src/ses/makeQ.js#L700> all

do. Within an engine, this technique should be straightforward to implement without slowing down the non-cyclic case.

# Dean Tribble (9 years ago)

I agree that the standard shoudl require a deterministic error, and I thought it did. In tc39.github.io/ecma262/#sec-promise-resolve-functions:

  1. If SameValue(resolution, promise) is true, then

a.Let selfResolutionError be a newly created TypeError object. b.Return RejectPromise(promise, selfResolutionError).

I suspect I assumed to much for "SameValue" here though. There's no well-defined other semantic answer to a cycle; it essentially always represents a bug, but could emerge dynamically out of bad code. You must either catch that as soon as possible or it's extremely difficult to isolate.

Another approach to efficiently achieve this in the implementation is to vigorously shorten targets. In this approach, the bf(a) would first shorten a so that it's internally pointing at the cell that bf will resolve to (chains are typically short, so keeping chains short is typically fast). Then the cycle check is simple and O(1). All the work is in shortening. There are some patterns that can make for interim long chains but they are straightforward to avoid.

# Raul-Sebastian Mihăilă (9 years ago)

When I first read that part of the spec, my understanding was that the two promises would cancel each other out by waiting for each other, but without performing an infinite set of PromiseJobs. The resolving functions created in step 1 of 25.4.2.2 that are set as reactions for the other promise are never triggered because the resolve function of the other promise never triggers the reactions, instead it just adds a set of resolving function itself for the other promise.

# Benjamin Gruenbaum (9 years ago)

For what it's worth, bluebird promises detect the error and reject with:

TypeError: Chaining cycle detected for promise #<Promise>

So it's both possible and not a performance issue. For this case, a WeakMap is not needed for this case. jsfiddle.net/41ez2b6d .

We ran into code "in the wild"

Yes, while I've never run into this I've talked to several people who have ran into this error - so it's definitely "a thing".

# Benjamin Gruenbaum (9 years ago)

Sorry, the example fiddle was before the inclusion of bluebird - I got confused by the UI. Here: jsfiddle.net/cm9kvLqv It appears like native chrome promises also detect it (although with a less informative error and only for one of the promises)

# Raul-Sebastian Mihăilă (9 years ago)

@Benjamin: Your example is different than Bradley's. Should be:

var r1, p1 = new Promise(r => r1 = r);

var r2, p2 = new Promise(r => r2 = r);
# Benjamin Gruenbaum (9 years ago)

Thanks​ Raul, I understand this case better now - fixed in master petkaantonov/bluebird/commit/7094e1677d79de99ba5f268785f49e9d99508e2f

  • wasn't particularly hard to fix this case, no one ever complained about it or mentioned it before so it wasn't considered.

Bluebird will now raise an error about a cyclical reference here.

Native promises should have no issue fixing this without any weak maps too.