Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Promise.prototype.finally to not incur extra ticks #1118

Closed
wants to merge 3 commits into from
Closed

Fix Promise.prototype.finally to not incur extra ticks #1118

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46c3b3d
Fix Promise.prototype.finally to not incur extra ticks
raulsebastianmihaila Feb 25, 2018
1701879
Forgot to delete some words
raulsebastianmihaila Feb 25, 2018
38fd8e8
Small improvement
raulsebastianmihaila Feb 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 32 additions & 63 deletions spec.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38532,14 +38532,15 @@ <h1>PromiseReaction Records</h1>

<!-- es6num="25.4.1.3" -->
<emu-clause id="sec-createresolvingfunctions" aoid="CreateResolvingFunctions">
<h1>CreateResolvingFunctions ( _promise_ )</h1>
<h1>CreateResolvingFunctions ( _promise_, _finallySourcePromise_ )</h1>
<p>When CreateResolvingFunctions is performed with argument _promise_, the following steps are taken:</p>
<emu-alg>
1. Let _alreadyResolved_ be a new Record { [[Value]]: *false* }.
1. Let _stepsResolve_ be the algorithm steps defined in Promise Resolve Functions (<emu-xref href="#sec-promise-resolve-functions"></emu-xref>).
1. Let _resolve_ be CreateBuiltinFunction(_stepsResolve_, &laquo; [[Promise]], [[AlreadyResolved]] &raquo;).
1. Let _resolve_ be CreateBuiltinFunction(_stepsResolve_, &laquo; [[Promise]], [[AlreadyResolved]], [[FinallySourcePromise]] &raquo;).
1. Set _resolve_.[[Promise]] to _promise_.
1. Set _resolve_.[[AlreadyResolved]] to _alreadyResolved_.
1. Set _resolve_.[[FinallySourcePromise]] to _finallySourcePromise_.
1. Let _stepsReject_ be the algorithm steps defined in Promise Reject Functions (<emu-xref href="#sec-promise-reject-functions"></emu-xref>).
1. Let _reject_ be CreateBuiltinFunction(_stepsReject_, &laquo; [[Promise]], [[AlreadyResolved]] &raquo;).
1. Set _reject_.[[Promise]] to _promise_.
Expand All @@ -38566,26 +38567,34 @@ <h1>Promise Reject Functions</h1>
<!-- es6num="25.4.1.3.2" -->
<emu-clause id="sec-promise-resolve-functions">
<h1>Promise Resolve Functions</h1>
<p>A promise resolve function is an anonymous built-in function that has [[Promise]] and [[AlreadyResolved]] internal slots.</p>
<p>A promise resolve function is an anonymous built-in function that has [[Promise]], [[AlreadyResolved]] and [[FinallySourcePromise]] internal slots.</p>
<p>When a promise resolve function _F_ is called with argument _resolution_, the following steps are taken:</p>
<emu-alg>
1. Assert: _F_ has a [[Promise]] internal slot whose value is an Object.
1. Assert: _F_ has a [[FinallySourcePromise]] internal slot whose value is either a promise object with a [[PromiseState]] internal slot or *undefined*.
1. Let _promise_ be _F_.[[Promise]].
1. Let _alreadyResolved_ be _F_.[[AlreadyResolved]].
1. If _alreadyResolved_.[[Value]] is *true*, return *undefined*.
1. Set _alreadyResolved_.[[Value]] to *true*.
1. If SameValue(_resolution_, _promise_) is *true*, then
1. Let _selfResolutionError_ be a newly created *TypeError* object.
1. Return RejectPromise(_promise_, _selfResolutionError_).
1. Let _finallySourcePromise_ be _F_.[[FinallySourcePromise]].
1. If Type(_resolution_) is not Object, then
1. Return FulfillPromise(_promise_, _resolution_).
1. If _finallySourcePromise_ is *undefined*
1. Return FulfillPromise(_promise_, _resolution_).
1. Else
1. If _finallySourcePromise_.[[PromiseState]] is "rejected"
1. Return RejectPromise(_promise_, _finallySourcePromise_.[[PromiseResult]]).
1. Else
1. Return FulfillPromise(_promise_, _finallySourcePromise_.[[PromiseResult]]).
1. Let _then_ be Get(_resolution_, `"then"`).
1. If _then_ is an abrupt completion, then
1. Return RejectPromise(_promise_, _then_.[[Value]]).
1. Let _thenAction_ be _then_.[[Value]].
1. If IsCallable(_thenAction_) is *false*, then
1. Return FulfillPromise(_promise_, _resolution_).
1. Perform EnqueueJob(`"PromiseJobs"`, PromiseResolveThenableJob, &laquo; _promise_, _resolution_, _thenAction_ &raquo;).
1. Perform EnqueueJob(`"PromiseJobs"`, PromiseResolveThenableJob, &laquo; _promise_, _resolution_, _thenAction_, _finallySourcePromise_ &raquo;).
1. Return *undefined*.
</emu-alg>
<p>The `length` property of a promise resolve function is 1.</p>
Expand All @@ -38609,15 +38618,17 @@ <h1>FulfillPromise ( _promise_, _value_ )</h1>

<!-- es6num="25.4.1.5" -->
<emu-clause id="sec-newpromisecapability" aoid="NewPromiseCapability">
<h1>NewPromiseCapability ( _C_ )</h1>
<p>The abstract operation NewPromiseCapability takes a constructor function, and attempts to use that constructor function in the fashion of the built-in `Promise` constructor to create a Promise object and extract its resolve and reject functions. The promise plus the resolve and reject functions are used to initialize a new PromiseCapability Record which is returned as the value of this abstract operation.</p>
<h1>NewPromiseCapability ( _C_ [ , _finallySourcePromise_ ] )</h1>
<p>The abstract operation NewPromiseCapability takes a constructor function, and attempts to use that constructor function in the fashion of the built-in `Promise` constructor to create a Promise object and extract its resolve and reject functions. The promise plus the resolve and reject functions are used to initialize a new PromiseCapability Record which is returned as the value of this abstract operation. NewPromiseCapability also takes an optional _finallySourcePromise_, which is either a promise object with a [[PromiseState]] internal slot or *undefined*. If _finallySourcePromise_ is a promise object, it is the promise object on which `Promise.prototype.finally` (<emu-xref href="#sec-promise.prototype.finally"></emu-xref>) was invoked, during which NewPromiseCapability was performed in order to create the resulted promise.</p>
<emu-alg>
1. If IsConstructor(_C_) is *false*, throw a *TypeError* exception.
1. NOTE: _C_ is assumed to be a constructor function that supports the parameter conventions of the `Promise` constructor (see <emu-xref href="#sec-promise-executor"></emu-xref>).
1. If _finallySourcePromise_ is not present, set _finallySourcePromise_ to *undefined*.
1. Let _promiseCapability_ be a new PromiseCapability { [[Promise]]: *undefined*, [[Resolve]]: *undefined*, [[Reject]]: *undefined* }.
1. Let _steps_ be the algorithm steps defined in <emu-xref href="#sec-getcapabilitiesexecutor-functions" title></emu-xref>.
1. Let _executor_ be CreateBuiltinFunction(_steps_, &laquo; [[Capability]] &raquo;).
1. Let _executor_ be CreateBuiltinFunction(_steps_, &laquo; [[Capability]] &raquo;, &laquo; [[FinallySourcePromise]] &raquo;).
1. Set _executor_.[[Capability]] to _promiseCapability_.
1. Set _executor_.[[FinallySourcePromise]] to _finallySourcePromise_.
1. Let _promise_ be ? Construct(_C_, &laquo; _executor_ &raquo;).
1. If IsCallable(_promiseCapability_.[[Resolve]]) is *false*, throw a *TypeError* exception.
1. If IsCallable(_promiseCapability_.[[Reject]]) is *false*, throw a *TypeError* exception.
Expand All @@ -38631,10 +38642,11 @@ <h1>NewPromiseCapability ( _C_ )</h1>
<!-- es6num="25.4.1.5.1" -->
<emu-clause id="sec-getcapabilitiesexecutor-functions">
<h1>GetCapabilitiesExecutor Functions</h1>
<p>A GetCapabilitiesExecutor function is an anonymous built-in function that has a [[Capability]] internal slot.</p>
<p>A GetCapabilitiesExecutor function is an anonymous built-in function that has a [[Capability]] and a [[FinallySourcePromise]] internal slots.</p>
<p>When a GetCapabilitiesExecutor function _F_ is called with arguments _resolve_ and _reject_, the following steps are taken:</p>
<emu-alg>
1. Assert: _F_ has a [[Capability]] internal slot whose value is a PromiseCapability Record.
1. Assert: _F_ has a [[FinallySourcePromise]] internal slot whose value is either a promise object with a [[PromiseState]] internal slot or *undefined*.
1. Let _promiseCapability_ be _F_.[[Capability]].
1. If _promiseCapability_.[[Resolve]] is not *undefined*, throw a *TypeError* exception.
1. If _promiseCapability_.[[Reject]] is not *undefined*, throw a *TypeError* exception.
Expand Down Expand Up @@ -38737,10 +38749,10 @@ <h1>PromiseReactionJob ( _reaction_, _argument_ )</h1>

<!-- es6num="25.4.2.2" -->
<emu-clause id="sec-promiseresolvethenablejob" aoid="PromiseResolveThenableJob">
<h1>PromiseResolveThenableJob ( _promiseToResolve_, _thenable_, _then_ )</h1>
<p>The job PromiseResolveThenableJob with parameters _promiseToResolve_, _thenable_, and _then_ performs the following steps:</p>
<h1>PromiseResolveThenableJob ( _promiseToResolve_, _thenable_, _then_, _finallySourcePromise_ )</h1>
<p>The job PromiseResolveThenableJob with parameters _promiseToResolve_, _thenable_, _then_, and _finallySourcePromise_ performs the following steps:</p>
<emu-alg>
1. Let _resolvingFunctions_ be CreateResolvingFunctions(_promiseToResolve_).
1. Let _resolvingFunctions_ be CreateResolvingFunctions(_promiseToResolve_, _finallySourcePromise_).
1. Let _thenCallResult_ be Call(_then_, _thenable_, &laquo; _resolvingFunctions_.[[Resolve]], _resolvingFunctions_.[[Reject]] &raquo;).
1. If _thenCallResult_ is an abrupt completion, then
1. Let _status_ be Call(_resolvingFunctions_.[[Reject]], *undefined*, &laquo; _thenCallResult_.[[Value]] &raquo;).
Expand Down Expand Up @@ -38771,7 +38783,10 @@ <h1>Promise ( _executor_ )</h1>
1. Set _promise_.[[PromiseFulfillReactions]] to a new empty List.
1. Set _promise_.[[PromiseRejectReactions]] to a new empty List.
1. Set _promise_.[[PromiseIsHandled]] to *false*.
1. Let _resolvingFunctions_ be CreateResolvingFunctions(_promise_).
1. Let _finallySourcePromise_ be *undefined*.
1. If _executor_ has a [[FinallySourcePromise]] internal slot, then
1. Set _finallySourcePromise_ to _executor_.[[FinallySourcePromise]].
1. Let _resolvingFunctions_ be CreateResolvingFunctions(_promise_, _finallySourcePromise_).
1. Let _completion_ be Call(_executor_, *undefined*, &laquo; _resolvingFunctions_.[[Resolve]], _resolvingFunctions_.[[Reject]] &raquo;).
1. If _completion_ is an abrupt completion, then
1. Perform ? Call(_resolvingFunctions_.[[Reject]], *undefined*, &laquo; _completion_.[[Value]] &raquo;).
Expand Down Expand Up @@ -39015,57 +39030,11 @@ <h1>Promise.prototype.finally ( _onFinally_ )</h1>
<p>When the `finally` method is called with argument _onFinally_, the following steps are taken:</p>
<emu-alg>
1. Let _promise_ be the *this* value.
1. If Type(_promise_) is not Object, throw a *TypeError* exception.
1. If IsPromise(_promise_) is *false*, throw a *TypeError* exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn’t going to work; finally must work with thenables.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that? Promise.prototype.then does this check as well. Shouldn't Promise.prototype.then work with thenables as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of subclassing, that check must be restricted to .then so that it can be overridden.

1. Let _C_ be ? SpeciesConstructor(_promise_, %Promise%).
1. Assert: IsConstructor(_C_) is *true*.
1. If IsCallable(_onFinally_) is *false*, then
1. Let _thenFinally_ be _onFinally_.
1. Let _catchFinally_ be _onFinally_.
1. Else,
1. Let _stepsThenFinally_ be the algorithm steps defined in <emu-xref href="#sec-thenfinallyfunctions" title></emu-xref>.
1. Let _thenFinally_ be CreateBuiltinFunction(_stepsThenFinally_, &laquo; [[Constructor]], [[OnFinally]] &raquo;).
1. Set _thenFinally_.[[Constructor]] to _C_.
1. Set _thenFinally_.[[OnFinally]] to _onFinally_.
1. Let _stepsCatchFinally_ be the algorithm steps defined in <emu-xref href="#sec-catchfinallyfunctions" title></emu-xref>.
1. Let _catchFinally_ be CreateBuiltinFunction(_stepsCatchFinally_, &laquo; [[Constructor]], [[OnFinally]] &raquo;).
1. Set _catchFinally_.[[Constructor]] to _C_.
1. Set _catchFinally_.[[OnFinally]] to _onFinally_.
1. Return ? Invoke(_promise_, `"then"`, &laquo; _thenFinally_, _catchFinally_ &raquo;).
</emu-alg>

<emu-clause id="sec-thenfinallyfunctions">
<h1>Then Finally Functions</h1>
<p>A Then Finally function is an anonymous built-in function that has a [[Constructor]] and an [[OnFinally]] internal slot. The value of the [[Constructor]] internal slot is a `Promise`-like constructor function object, and the value of the [[OnFinally]] internal slot is a function object.</p>
<p>When a Then Finally function _F_ is called with argument _value_, the following steps are taken:</p>
<emu-alg>
1. Let _onFinally_ be _F_.[[OnFinally]].
1. Assert: IsCallable(_onFinally_) is *true*.
1. Let _result_ be ? Call(_onFinally_, *undefined*).
1. Let _C_ be _F_.[[Constructor]].
1. Assert: IsConstructor(_C_) is *true*.
1. Let _promise_ be ? PromiseResolve(_C_, _result_).
1. Let _valueThunk_ be equivalent to a function that returns _value_.
1. Return ? Invoke(_promise_, `"then"`, &laquo; _valueThunk_ &raquo;).
</emu-alg>
<p>The `length` property of a Then Finally function is *1*.</p>
</emu-clause>

<emu-clause id="sec-catchfinallyfunctions">
<h1>Catch Finally Functions</h1>
<p>A Catch Finally function is an anonymous built-in function that has a [[Constructor]] and an [[OnFinally]] internal slot. The value of the [[Constructor]] internal slot is a `Promise`-like constructor function object, and the value of the [[OnFinally]] internal slot is a function object.</p>
<p>When a Catch Finally function _F_ is called with argument _reason_, the following steps are taken:</p>
<emu-alg>
1. Let _onFinally_ be _F_.[[OnFinally]].
1. Assert: IsCallable(_onFinally_) is *true*.
1. Let _result_ be ? Call(_onFinally_, *undefined*).
1. Let _C_ be _F_.[[Constructor]].
1. Assert: IsConstructor(_C_) is *true*.
1. Let _promise_ be ? PromiseResolve(_C_, _result_).
1. Let _thrower_ be equivalent to a function that throws _reason_.
1. Return ? Invoke(_promise_, `"then"`, &laquo; _thrower_ &raquo;).
</emu-alg>
<p>The `length` property of a Catch Finally function is *1*.</p>
</emu-clause>
1. Let _resultCapability_ be ? NewPromiseCapability(_C_, _promise_).
1. Return PerformPromiseThen(_promise_, _onFinally_, _onFinally_, _resultCapability_).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that this does not observably call .then immediately means that a subclass has to override .finally - with the current design, it only has to override .then (which is a requirement)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense because in essence then and finally are different. Could you please clarify why you think otherwise? The current design is based on what I consider to be a workaround instead of being an exact expression of what finally means conceptually, hence the extra ticks that finally doesn't conceptually require.
I think finally should call then only if the behavior expressed by then is part of finally's behavior. In my opinion it's not fully; they are similar only to a certain degree. Perhaps listing the pros and cons and the possible trade-offs and analyzing which one is better would help making the right choice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm saying applies to Promise.prototype.catch as well because Promise.prototype.then includes the 'catch' behavior, so the catch method is just another way of calling then. finally is essentially different.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, you mean that if the subclass overrides then, it must then override finally as well, is that correct? Because a subclass doesn't have to override then unless it chooses to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, no one likes promise subclassing anyway :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general consensus in the room - or at least, the only way to achieve consensus, which is effectively the same - was that either a) .catch and .finally should both directly delegate to .then, or, b) .catch and .finally should both call abstract operations directly. "b" is not web-compatible.

I'm not going to enumerate which committee members were blockers there, both to not throw folks under the bus and because it's irrelevant (the notes are not transcripts, intentionally) - I hope you can simply trust that what I've said above is the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we phrase it so that engines have to implement finally through then only for non-subclasses? That would be sufficient - subclassing is almost never used anyway since async functions return the base class always anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Engines can already optimize out the .then calls when it's not a subclass and when Promise.prototype.then is unmodified; but no, if I replace Promise.prototype.then it needs to be able to observe the calls too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb I don't doubt what you're saying but I also can't help notice that the motivation for why those 2 options were the only ones considered by TC39 is missing. If TC39 isn't willing to evaluate (or re-evaluate, in case they already have) the aspects I've mentioned, this PR is DOA.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third option is what this PR does, which is what the finally proposal did originally, and what the committee rejected - see tc39/proposal-promise-finally#34

</emu-alg>
</emu-clause>

<!-- es6num="25.4.5.3" -->
Expand Down