Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

(v1.0) Postpone coercion to time-of-use #372

Closed
wants to merge 3 commits into from
Closed

(v1.0) Postpone coercion to time-of-use #372

wants to merge 3 commits into from

Conversation

kriskowal
Copy link
Owner

Regarding #369

Among other things, if Q wraps a thenable, this postpones coercion until
the consumer attempts to use the promise. Before, Q(thenable) would
call thenable.then as soon as possible, but now the user must go all
the way to Q(thenable).then() in order for thenable.then() to be
invoked. This is less surprising in principle.

This is achived by adding an "accepted" state to deferred promises,
between "pending" and "resolved". This state is reflected by
deferred.promise.inspect().

This change breaks many specs, which need to be reviewed and probably
fixed.

Regarding #369

Among other things, if Q wraps a thenable, this postpones coercion until
the consumer attempts to use the promise.  Before, `Q(thenable)` would
call `thenable.then` as soon as possible, but now the user must go all
the way to `Q(thenable).then()` in order for `thenable.then()` to be
invoked.  This is less surprising in principle.

This is achived by adding an "accepted" state to deferred promises,
between "pending" and "resolved".  This state is reflected by
`deferred.promise.inspect()`.

This change breaks many specs, which need to be reviewed and probably
fixed.
@domenic
Copy link
Collaborator

domenic commented Aug 27, 2013

I dislike exposing accepted as a state. Unfortunately I am not sure I know of a way around it. Perhaps inspect() could trigger the unwrapping process? But no, access to untrusted "then" methods must be done in a fresh stack, so that would never be useful. Sigh. It definitely complicates the pedagogical story.

@kriskowal
Copy link
Owner Author

I believe @erights may want to review these changes and contemplate the implications.

@erights
Copy link
Collaborator

erights commented Aug 27, 2013

At the .then-level, which is the level Q and Promises/A+ operate at (in contrast to the lower .flatMap-level of the full AP2 proposal on es-discuss), we still don't need to distinguish AP2's adoption from acceptance, so I would avoid the term "accepted". Rather, if promise p is resolved to promise q, then p follows q. The question is, how to describe the state where p is resolved to a non-promise v, where we don't yet even know whether v will be considered thenable. In this case, at the AP2 .flatMap-level p can only accept v, since only promises can be adopted. Nevertheless, at our .then-level, I think the right concept to extend is "following" as in p follows v. So a promise can follow anything. But once p.then happens, in a later turn we further resolve p according to v's state and behavior.

As Domenic says, as long as v might be a thenable, "fulfilled" and "rejected" are no longer concrete states, but rather expository states to explain how p's behavior depends on v's.

Regarding the Q-specific inspection methods, I doubt there is any perfect way to maintain compatibility with their old behavior. So let's start by asking: What are these used for?

@kriskowal
Copy link
Owner Author

@erights Debugging, testing, and optimizing are the uses today. It is also the polymorphic basis for implementing methods like nearer, mostResolved, and the deprecated valueOf.

@kriskowal
Copy link
Owner Author

I should clarify. nearer is our implementation of shorten described on the Concurrency Strawman and receives very little use. mostResolved does not exist, but I have contemplated reimplementing nearer in terms of mostResolved. mostResolved would be responsible for following the promise chain to the last promise, and nearer or shorten would be responsible for unpacking the fulfillment value from the most resolved promise.

@erights
Copy link
Collaborator

erights commented Aug 27, 2013

What would happen if we withdrew all of these for a while and then added them, or something like them, back in as we encounter concrete use cases that need them?

@kriskowal
Copy link
Owner Author

I would be content to dump shorten, nearer, mostResolved.

inspect is used to optimize all and allSettled, as well as backing isFulfilled, isRejected, and isPending. We could stretch the pending state to cover accepted and keep things mostly working.

@kriskowal
Copy link
Owner Author

These are the affected tests if we stretch accepted over pending.

Failures:

  1) inspect for a promise resolved to a rejected promise
   Message:
     Expected { state : 'pending' } to equal { state : 'rejected', reason : {  } }.
   Stacktrace:
     Error: Expected { state : 'pending' } to equal { state : 'rejected', reason : {  } }.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:817:44)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  2) inspect for a promise resolved to a fulfilled promise
   Message:
     Expected { state : 'pending' } to equal { state : 'fulfilled', value : 10 }.
   Stacktrace:
     Error: Expected { state : 'pending' } to equal { state : 'fulfilled', value : 10 }.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:828:44)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  3) promise states of deferred rejection
   Message:
     Expected false to be true.
   Stacktrace:
     Error: Expected false to be true.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:891:38)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  4) promise states of deferred rejection
   Message:
     Expected true to be false.
   Stacktrace:
     Error: Expected true to be false.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:892:37)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  5) promise states of deferred fulfillment
   Message:
     Expected false to be true.
   Stacktrace:
     Error: Expected false to be true.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:899:39)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  6) promise states of deferred fulfillment
   Message:
     Expected true to be false.
   Stacktrace:
     Error: Expected true to be false.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:901:37)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  7) promise states of isFulfilled side effects
   Message:
     Expected true to be false.
   Stacktrace:
     Error: Expected true to be false.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:937:46)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  8) promise states of isFulfilled side effects
   Message:
     Expected false to be true.
   Stacktrace:
     Error: Expected false to be true.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:939:48)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  9) promise states of isFulfilled side effects
   Message:
     Expected undefined to be 2.
   Stacktrace:
     Error: Expected undefined to be 2.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:940:50)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  10) all rejects after any constituent promise is rejected
   Message:
     Expected false to be true.
   Stacktrace:
     Error: Expected false to be true.
    at /Users/kris/q/spec/q-spec.js:1102:42
    at _fulfilled (/Users/kris/q/q.js:794:54)
    at self.promiseDispatch.done (/Users/kris/q/q.js:823:30)
    at Promise.promise.promiseDispatch (/Users/kris/q/q.js:756:13)
    at /Users/kris/q/q.js:580:53
    at flush (/Users/kris/q/q.js:108:17)
    at process._tickDomainCallback (node.js:459:13)

  11) delay should not delay rejection
   Message:
     Expected true to be false.
   Stacktrace:
     Error: Expected true to be false.
    at /Users/kris/q/spec/q-spec.js:1659:41
    at _fulfilled (/Users/kris/q/q.js:794:54)
    at self.promiseDispatch.done (/Users/kris/q/q.js:823:30)
    at Promise.promise.promiseDispatch (/Users/kris/q/q.js:756:13)
    at /Users/kris/q/q.js:580:53
    at flush (/Users/kris/q/q.js:108:17)
    at process._tickDomainCallback (node.js:459:13)

  12) unhandled rejection reporting doesn't report a resolve, then reject (gh-252)
   Message:
     Expected 1 to equal 0.
   Stacktrace:
     Error: Expected 1 to equal 0.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:2443:48)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

Finished in 2.119 seconds
180 tests, 287 assertions, 12 failures, 0 skipped

@kriskowal
Copy link
Owner Author

If we promote acceptance to adoption at time of resolution if the accepted value is a branded promise, the fallout drops somewhat.

At the end of the day, I would not mind dumping isFulfilled, isRejected, and isPending, and all of the optimizations that use inspect. inspect could disappear entirely. I expect the only people who would be saddened would be the users—I say only somewhat facetiously.

Failures:

  1) promise states of deferred fulfillment
   Message:
     Expected false to be true.
   Stacktrace:
     Error: Expected false to be true.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:899:39)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  2) promise states of deferred fulfillment
   Message:
     Expected true to be false.
   Stacktrace:
     Error: Expected true to be false.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:901:37)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

  3) promise states of isFulfilled side effects
   Message:
     Expected true to be false.
   Stacktrace:
     Error: Expected true to be false.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:937:46)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  4) promise states of isFulfilled side effects
   Message:
     Expected false to be true.
   Stacktrace:
     Error: Expected false to be true.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:939:48)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  5) promise states of isFulfilled side effects
   Message:
     Expected undefined to be 2.
   Stacktrace:
     Error: Expected undefined to be 2.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:940:50)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)
    at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  6) unhandled rejection reporting doesn't report a resolve, then reject (gh-252)
   Message:
     Expected 1 to equal 0.
   Stacktrace:
     Error: Expected 1 to equal 0.
    at null.<anonymous> (/Users/kris/q/spec/q-spec.js:2443:48)
    at jasmine.Block.execute (/Users/kris/q/spec/lib/jasmine-promise.js:21:32)

Finished in 2.126 seconds
180 tests, 287 assertions, 6 failures, 0 skipped

@erights
Copy link
Collaborator

erights commented Aug 27, 2013

This question clarified something for me regarding thenables and memoization. With apologies to Albert, I propose that "Thenables should be assimilated as late as possible, but no later." Once p.then(v => .., e =>..) fires, from then on it remains fulfilled or rejected to whatever it provided as the callback argument. In particular, if v later becomes a thenable, this will not affect the behavior of p itself. A later p.then will still report p as fulfilled with v itself. Or, with apologies to post-Albert, observing p's settlement (by being invoked as a callback by p.then) causes the collapse of p's assimilation function.

@kriskowal
Copy link
Owner Author

I’ll leave it to our resident Caltech physicist to put to record whether the uncertainty metaphor applies as well as it sounds!

@erights
Copy link
Collaborator

erights commented Aug 27, 2013

I asked my cat and she says "well, yes and no."

@domenic
Copy link
Collaborator

domenic commented Aug 27, 2013

lol!

The memoization plan makes sense to me, as does the analogy :). Sigh, now to spec all this...

@domenic
Copy link
Collaborator

domenic commented Aug 27, 2013

BTW @kriskowal would love your feedback on https://github.com/domenic/promises-unwrapping, which tries to capture this late-unwrapping shift. It still needs memoization work though.

domenic added a commit to domenic/promises-unwrapping that referenced this pull request Aug 27, 2013
kriskowal added a commit that referenced this pull request Oct 21, 2013
Implement the `Promise` constructor.  The promise constructor serves
both as a deferred promise constructor that accepts a function, and a
new kind of promise constructor that accepts a backing handler object.
The backing handler object must implement `dispatch(resolve, op,
operands)` and `inspect()`.  The new promise constructor replaces the
`Q.promise` function, which is deprecated.  The new promise constructor
replaces `makePromise`, which has been removed entirely.  As such,
Q-Connection will have to be rearchitected to provide a custom promise
handler for remote objects instead of using `makePromise`. Fixes #346.

Postpone calling `then` on a thenable until a message is dispatched to
the coerced promise.  Fixes #372.

When coercing a thenable, memoize the resulting promise to avoid
re-starting a lazy promise.

Add support for vicious cycle detection.  Fixes #223.

This change request also reviews the Q API, deprecating many interfaces
that remain from legacy designs.  Fixes #215.

Factor most Node.js tools into `q/node` module.  Mirror deprecated
interfaces in Q.

Support for `close` and `closed` has been removed from `Queue`, which
has additional ramifications for Q-Connection.  I intend to use Q-IO
streams in Q-Connection instead of raw queues.

Most of the Q specifications continue to work after these changes, but
with many deprecation warnings.  The specs have been revised to appease
the deprecation warnings.

:warning: However, the specifications for "progress" have all been
disabled pending a closer investigation to decide whether to fix Q or
fix the specs.

The promise protocol no longer supports "set" and "delete" operations.
Function application is a special case of "post", and for support of
"fbind", it is now possible to pass a "thisp" as a final argument.  The
"when" message is now called simply "then".

Support for pre-ECMAScript 5 has been abandoned outright, pending
review.

Removed:

-   Q.set, promise.set
-   Q.delete, promise.delete
-   Q.nearer
-   Q.master

The following methods of `Q` are deprecated in favor of their
equivalents on the `promise` prototype:

-   `progress`, `thenResolve`, `thenReject`, `isPending`, `isFulfilled`,
    `isRejected`, `dispatch`, `get`, `post`, `invoke`, `keys`

Other deprecations:

-   Q.resolve in favor of Q
-   Q.fulfill in favor of Q
-   Q.isPromiseAlike in favor of Q.isThenable
-   Q.when in favor of Q().then
-   Q.fail and promise.fail in favor of promise.catch
-   Q.fin and promise.fin in favor of promise.finally
-   Q.mapply and promise.mapply in favor of promise.post
-   Q.send and promise.send in favor of promise.invoke
-   Q.mcall and promise.mcall in favor of promise.invoke
-   Q.promise in favor of new Q.Promise with a resolver function
-   Q.makePromise in favor of new Q.Promise with a handler object
-   promise.fbind in favor of Q.fbind
-   deferred.makeNodeResolver() in favor of
    require("q/node").makeNodeResolver(deferred.resolve)
-   promise.passByCopy() in favor of Q.passByCopy(promise),
    provisionally

Node.js wrappers that have been moved into their own module have a
deprecated interface in Q proper:

-   `nodeify`, `denodify`, `nfbind`, `nbind`, `npost`, `ninvoke`

But the following experimental aliases are deprecated and do not exist
in `q/node`:

-   `nsend` for `ninvoke`
-   `nmcall` for `ninvoke`
-   `nmapply` for `npost`
kriskowal added a commit that referenced this pull request Oct 30, 2013
Implement the `Promise` constructor.  The promise constructor serves
both as a deferred promise constructor that accepts a function, and a
new kind of promise constructor that accepts a backing handler object.
The backing handler object must implement `dispatch(resolve, op,
operands)` and `inspect()`.  The new promise constructor replaces the
`Q.promise` function, which is deprecated.  The new promise constructor
replaces `makePromise`, which has been removed entirely.  As such,
Q-Connection will have to be rearchitected to provide a custom promise
handler for remote objects instead of using `makePromise`. Fixes #346.

Postpone calling `then` on a thenable until a message is dispatched to
the coerced promise.  Fixes #372. Fixes #369.

When coercing a thenable, memoize the resulting promise to avoid
re-starting a lazy promise.

Add support for vicious cycle detection.  Fixes #223.

This change request also reviews the Q API, deprecating many interfaces
that remain from legacy designs.  Fixes #215.

Factor most Node.js tools into `q/node` module.  Mirror deprecated
interfaces in Q.

Support for `close` and `closed` has been removed from `Queue`, which
has additional ramifications for Q-Connection.  I intend to use Q-IO
streams in Q-Connection instead of raw queues.

Most of the Q specifications continue to work after these changes, but
with many deprecation warnings.  The specs have been revised to appease
the deprecation warnings.

:warning: However, the specifications for "progress" have all been
disabled pending a closer investigation to decide whether to fix Q or
fix the specs.

The promise protocol no longer supports "set" and "delete" operations.
Function application is a special case of "post", and for support of
"fbind", it is now possible to pass a "thisp" as a final argument.  The
"when" message is now called simply "then".

Support for pre-ECMAScript 5 has been abandoned outright, pending
review.

Removed:

-   Q.set, promise.set
-   Q.delete, promise.delete
-   Q.nearer
-   Q.master

The following methods of `Q` are deprecated in favor of their
equivalents on the `promise` prototype:

-   `progress`, `thenResolve`, `thenReject`, `isPending`, `isFulfilled`,
    `isRejected`, `dispatch`, `get`, `post`, `invoke`, `keys`

Other deprecations:

-   Q.resolve in favor of Q
-   Q.fulfill in favor of Q
-   Q.isPromiseAlike in favor of Q.isThenable
-   Q.when in favor of Q().then
-   Q.fail and promise.fail in favor of promise.catch
-   Q.fin and promise.fin in favor of promise.finally
-   Q.mapply and promise.mapply in favor of promise.post
-   Q.send and promise.send in favor of promise.invoke
-   Q.mcall and promise.mcall in favor of promise.invoke
-   Q.promise in favor of new Q.Promise with a resolver function
-   Q.makePromise in favor of new Q.Promise with a handler object
-   promise.fbind in favor of Q.fbind
-   deferred.makeNodeResolver() in favor of
    require("q/node").makeNodeResolver(deferred.resolve)
-   promise.passByCopy() in favor of Q.passByCopy(promise),
    provisionally

Node.js wrappers that have been moved into their own module have a
deprecated interface in Q proper:

-   `nodeify`, `denodify`, `nfbind`, `nbind`, `npost`, `ninvoke`

But the following experimental aliases are deprecated and do not exist
in `q/node`:

-   `nsend` for `ninvoke`
-   `nmcall` for `ninvoke`
-   `nmapply` for `npost`
@kriskowal kriskowal closed this in 67cfb6e Oct 30, 2013
@kriskowal kriskowal deleted the accept branch December 4, 2013 20:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants