Don't test promise results on their thenability multiple times
# Bergi (11 years ago)
Another, maybe simpler way to put this:
Promise.resolve needs to become idempotent, i.e. for all values x
the expression
Promise.resolve(x)
must behave exactly as the expression
Promise.resolve(Promise.resolve(x));
where it accesses x.then only exactly once. The current spec would
test for it twice, which even gives way to fulfilling promises with
thenables - if only the then method is attached to your object after
you have resolved your promise with it. Which could cause some quite
unexpected results.
Bergi
Hello, while working on the promise memory-leak problem promises-aplus/promises-spec#179 I noticed the following in the current ES6 spec:
var count = 0, nonThenable = {get then() { count++; return undefined; }}; function GetResult(n) { if (n <= 0) return nonThenable; return Promise.resolve().then(()=> // just for a little delay GetResult(n-1); ); } GetResult(5).then((res) => { assert(res == nonThenable); assert(count == 1); // !!! })Since we only resolved a promise once with
nonThenable(the promise that is returned fromGetResult(1)), I expect that.thenis only accessed once. The other 4 promises are fulfilled with it, and we know that [[PromiseResult]]s are never thenables. Or should not be, if it didn't change in the meantime.I suggest to change the spec so that a Promise Resolve Function (25.4.1.3.2) is not called multiple times, even if multiple promises (that adopt each other) are fulfilled with it. This would pave the road to allow fixing memory leaks from long recursive adoption chains, so that "intermediate" promises are no longer considered and can be garbage-collected before the eventual resolution.
A possible change could be made by amending the sections 25.4.1.3 CreateResolvingFunctions and 25.4.5.3 Promise.prototype.then like so:
A promise resolve function is an anonymous built-in function that has [[Promise]], [[AlreadyResolved]] and [[fulfillAdopter]] internal slots.
CreateResolvingFunctions would set the [[fulfillAdopter]] slot of
resolveto a function like function fulfiller(resul) { if (! F.[[AlreadyResolved]]) { F.[[AlreadyResolve]] = true Return FulfillPromise(F.[[Promise]], result) } }Insert a step 5.5 in PerformPromiseThen, like If
onFulfilledhas a [[fulfillAdopter]] internal slot, set fulfillReaction.[[handler]] to onFulfilled.[[fulfillAdopter]]So when a promise is adopted, and its
.then()method is called, afulfillhandler is installed not aresolvehandler. Which works as long as the invariant holds that result values are not thenable.I haven't thought through the implications that this had on subclassing, but this is supposed to work even if
.thenwas overwritten as long as it callssuper.then()with the original resolve handler. (If it wrapped the resolve handler, we should resolve the value again indeed)., Bergi
Hello, while working on the promise memory-leak problem <https://github.com/promises-aplus/promises-spec/issues/179> I noticed the following in the current ES6 spec: ```js var count = 0, nonThenable = {get then() { count++; return undefined; }}; function GetResult(n) { if (n <= 0) return nonThenable; return Promise.resolve().then(()=> // just for a little delay GetResult(n-1); ); } GetResult(5).then((res) => { assert(res == nonThenable); assert(count == 1); // !!! }) ``` Since we only *resolved* a promise once with `nonThenable` (the promise that is returned from `GetResult(1)`), I expect that `.then` is only accessed once. The other 4 promises are *fulfilled* with it, and we know that [[PromiseResult]]s are never thenables. Or should not be, if it didn't change in the meantime. I suggest to change the spec so that a Promise Resolve Function (25.4.1.3.2) is not called multiple times, even if multiple promises (that adopt each other) are fulfilled with it. This would pave the road to allow fixing memory leaks from long recursive adoption chains, so that "intermediate" promises are no longer considered and can be garbage-collected before the eventual resolution. A possible change could be made by amending the sections 25.4.1.3 CreateResolvingFunctions and 25.4.5.3 Promise.prototype.then like so: * A promise resolve function is an anonymous built-in function that has [[Promise]], [[AlreadyResolved]] *and [[fulfillAdopter]]* internal slots. * CreateResolvingFunctions would set the [[fulfillAdopter]] slot of `resolve` to a function like function fulfiller(resul) { if (! F.[[AlreadyResolved]]) { F.[[AlreadyResolve]] = true Return FulfillPromise(F.[[Promise]], result) } } * Insert a step 5.5 in PerformPromiseThen, like If `onFulfilled` has a [[fulfillAdopter]] internal slot, set fulfillReaction.[[handler]] to onFulfilled.[[fulfillAdopter]] So when a promise is adopted, and its `.then()` method is called, a `fulfill` handler is installed not a `resolve` handler. Which works as long as the invariant holds that result values are not thenable. I haven't thought through the implications that this had on subclassing, but this is supposed to work even if `.then` was overwritten as long as it calls `super.then()` with the original resolve handler. (If it wrapped the resolve handler, we should resolve the value again indeed). regards, Bergi