slightlyoff / Promises

DOM Promises IDL/polyfill
Apache License 2.0
154 stars 28 forks source link

semantics of "is-a Future" #41

Open dherman opened 11 years ago

dherman commented 11 years ago

Currently the prolyfill uses this code for testing whether a value is a Future:

!!x && typeof x.then === 'function' && x.then.length === 2

The issue with "duck testing" is that, as @domenic raised in Issue #13, the type of a fulfilled future is not fully polymorphic: you must never fulfill with a future that you intended to be a completed value. Bottom line: there needs to be a super-clear and ideally super-simple definition of what it means to be a future.

I'll pitch a few alternatives here, but I make no claim to exhaustiveness. Other suggestions welcome.

Approach: a "Future" [[Class]] with an isFuture test

Using instanceof is traditionally brittle because when multiple frames interact, they can have multiple, mutually incompatible versions of a given "class." That's why ES5 introduced Array.isArray as a way of testing whether something has the [[Class]] "Array", which is robust across frames, even when they have different Array constructors. So an analogous approach would be to introduce a new "Future" [[Class]] and an isFuture test that inspects the class. This is very clear and very robust, but has the downside that you can't create futures conveniently with an object literal.

Approach: a Symbol to definitely brand futures

Another approach would be to create a unique symbol (a new ES6 feature for creating opaque unforgeable property keys) that's shared in the standard library and identical across all frames. Then to create a future you would have to have this symbol, a la:

var myFuture = {
    [futureSymbol]: true,
    then: function(/* ... */) { /* ... */ },
    // ...
};

Approach: a string key to "probabilistically" brand futures

A lower-tech approach is to have some sort of published key that is simply a standard string key, but perhaps distinctive enough that it's highly unlikely (though still possible) that someone could accidentally create one. For example, Promises/A+ discussed alternatives like p.then.aplus or p.isPromise.

This approach is certainly the easiest, but I would hope we could do better when we have the opportunity to define this in the web platform and/or ES standard. What's particularly galling about it is that it effectively declares that whatever public key it's using is once and for all permanently associated with this type—that's global pollution of the entire JavaScript language. Scumbag futures!

Approach: type-and-arity check (again, probabilistic)

We could always just do what the prolyfill currently does. Again, while it's unlikely that someone would accidentally fulfill a value that is not intended to be a future but looks like one, that doesn't mean it can't happen. For example, you might be writing future-aware code that nevertheless fulfills with the result of calling some third-party code, and passes it back over to some other third-party code. And those third-parties may not be future-aware. If they happen to be using values that look like futures, not only will the program be buggy, it'll be buggy in badly unpredictable ways.

See also:

Dave

domenic commented 11 years ago

@dherman, re:

Approach: a string key to "probabilistically" brand futures

I don't see how

import { promiseBrand } from "promise";
assert(promise && promise[promiseBrand]);

is better than

assert(promise && promise.then && promise.then.aplus);

Obviously promise.isPromise isn't good. I'd argue that isPromise, and the current then test, are unpredictable and namespace-hogging. But promise.then.aplus seems equivalent to a symbol-based solution. It is just as hard to construct something that passes the latter test as it is to construct something that passes the former test.


The issue with "duck testing" is that, as @domenic raised in Issue #13, the type of a fulfilled future is not fully polymorphic: you must never fulfill with a future that you intended to be a completed value.

I think this might not be worded that clearly, i.e. it confuses the issues of return versus resolve/fulfill. It doesn't substantially impact the larger question of branding though, I just wanted to clarify.

In @erights's formulation, and in Promises/A+ (modulo our upcoming promise-creation spec), only return and resolve are present, and it is thus impossible to create a promise-for-promise---or more generally, a promise-for-thenable. In DOMFuture, the existence of the accept callback (which fulfills the future) allows the creation of DOMFutures-for-thenables, so it is susceptible to the hazards outlined in #13.

kriskowal commented 11 years ago

Thanks for breaking down the dimensions of the problem. I will argue for simple duck-typing.

It is equally bad in practice to fail to recognize something that is a promise, or at least close enough to being a promise that it can be coerced, as it is to accidentally attempt to coerce an object that is not a promise.

If an object has a "then" method, it is a close enough approximation to a promise that we can reliably coerce it into a good promise, even with very bad historical promise implementations.

If an object has a "then" method but is not a promise and has to be passed through a promise implementation, it is simple enough to make it a property of a wrapper object.

@dherman mentions this case where Alice and Bob are third party libraries that do not know of each other.

alice().then(function (a) {
    return bob(a);
})

Note that Alice and Bob are both necessarily aware that they are in the thenable promise ecosystem. Even today, they would both be aware of the peril.

Another case is where Alice is coordinating Bob and Charlie, where Bob is providing objects with then methods that are not promises and Charlie is consuming promises. This puts Alice in a position where she would know to wrap Bob’s objects such that Charlie can consume them. For example, this would occur if one were to pass @polotek’s process wrappers (which have a "then" method but are not promises) to a promise consumer. In this kind of case, there is always an arbiter (Alice) that can mitigate the problem.

To change the landscape will require a pretty significant amount of collective political will and a synchronized abandonment of existing implementations, including prollyfills.

The arity check is too strong. Promises/A has an arity of 3 and Promises/A+ has an arity of 2. Then there are forwarders that have an arity of 0.

Note that any symbol or sentinel value would have to be global enough that every promise implementation, even multiple instances of identical promise implementations, can recognize each other’s promises, and that every promise implementation would need to look for it in the same place, and for migration purposes, install it if it does not exist.

There is also no migration path from promises of today and promises that use symbols or modules to solve the problem.

The only solution that does not use duck typing is to make migration a non-goal.

dherman commented 11 years ago

@domenic I think the details come down to how specific the string-key name is. If it's aplus then, sure, it's probably pretty unlikely anyone else will have a function with an aplus expando property on it. If we choose a more generic name like future the probabilities change. Probably! This is kind of crystal-ball gazing. But in principle, by using public names/shapes you're effectively declaring "anyone who ever creates a value of this shape that isn't a promise is breaking the contract of the promises library." In order to be able to make plausible claim to some particular shape, you have to pick a sufficiently weird shape so as to be unlikely to step on the toes of some other library that wants to do something else. You're fighting for turf in a single universal sandbox.

Whereas, if the promise library itself creates and publishes a whole new value, then the library has the right to document the purpose of that value. It's not possible for someone to accidentally create a value with that brand without knowledge of the promises library, because they had to get it from the promises library! And by using the library they're agreeing to obey its contract. If they choose to violate the contract, it's their own fault if fulfilling does something weird.

Dave

dherman commented 11 years ago

@kriskowal A number of good points there. In particular I agree that the arity check is a no go. But I disagree that "anything with a then method will behave like a promise." The name then is so generic, and the behavior of promises so particular, that it's not hard to come up with examples that would break. First example off the top of my head: you could be writing an interpreter for some language with if-then-else nodes, where AST nodes are represented as functions that self-evaluate. That's no promise!

At the end of the day, I agree that it's possible to code your way around any of this, by being careful to wrap values that might possibly conflict with promises. In practice, I expect no one would actually do this, and most code would be susceptible to these bugs. They're just rare enough that no one will bother, and yet when they do happen they'll be really bad. And if you do try to write more robust code you'll pay a performance penalty for it.

Your points about migration and interoperability are good. Even if the web platform publishes a nonce that everyone can use, legacy code won't be able to use it, and it may not be possible to polyfill. I guess I'm not yet convinced that it's impossible to address migration and branding, but I'd have to think about it.

Dave

domenic commented 11 years ago

@dherman the main point I was making was that accidents are possible with one-level-deep brands, e.g. promise.isPromise or promise.then, whereas two-level-deep brands like promise.then.aplus or promise.then.promise should be as unlikely to arise by accident as promise[promiseSymbol]. (And the latter could arise by accident, of course: e.g. metaprogramming that lists all the exports of the ES standard library, and ends up putting them on an object, perhaps mapping them to human-readable names. This seems no more contrived than any scenario that ends up with an object that has a then property that is a function that has an aplus property that is a boolean with value true.)

domenic commented 11 years ago

I want to repeat a point from over in https://github.com/slightlyoff/DOMFuture/issues/40#issuecomment-14580464 that I don't see being addressed. If there is branding, it would mean that the following code does not work:

var promise = aplusCompliantPromise.then(() => jQueryPromise);

var promise = new Promise(resolve => resolve(jQueryPromise));

i.e. the promise becomes fulfilled with jQueryPromise, and does not adopt the fulfillment value or rejection reason of jQueryPromise.

Instead you need to provide an assimilation function, e.g.

var promise = aplusCompliantPromise.then(() => Promise.from(jQueryPromise));

var promise = new Promise(resolve => resolve(Promise.from(jQueryPromise)));

I want to make sure everyone who is pro-branding recognizes this cost and thinks it is worth it. I am on the fence, personally (and thus leaning toward the status quo of no-branding).

dherman commented 11 years ago

@domenic Good points all around. The latter point makes me sad, and I think it's about the same point @kriskowal is making: "like it or not, it's the cowpath."

If I lose this argument I suppose I'll live to see another day, I just may have to curse it along with __proto__ and every other design decision I don't like that we're stuck with on the web. ;-P

Dave

erights commented 11 years ago

A quick note, written only after skimming since I need to leave in a moment.

From skimming, this thread seems to be missing the crucial distinction between thenables and promises (ka "futures"). Promises should be recognized only by unforgeable branding/trademarking. But thenables, by definition, can (and should) only be recognized by duck typing. The only place where a promise library should duck type to recognize a thenable is when deciding whether to assimilate. https://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/ses/makeQ.js#204 does both of these correctly.

dherman commented 11 years ago

@erights That doesn't affect my argument. My point is that making a decision to recognize a thenable is the problem. Apologies for using the wrong terminology.

Dave

erights commented 11 years ago

I have now read through this thread carefully, and I think we're still missing the distinction between promises (aka futures) and thenables. @dherman I agree that the points you raise apply even just to the assimilation test by itself. But in the context where promises are reliably branded, the issue is much less bad than others might imagine reading this thread.

@dherman @domenic @kriskowal @slightlyoff The branding must be a reliable indicator that the value in question obeys the promise contract. @dherman writes: "If they choose to violate the contract, it's their own fault if fulfilling does something weird." Let's rephrase that as "If an attacker chooses to violate the contract, it's their own fault if that misleads the victim into doing something weird." Let's remember where this promise design originated and what its original purpose was: to protect the invoker/victim from the plan interference hazards that arise from caller/callee interleaving http://www.erights.org/talks/promises/paper/tgc05.pdf http://www.infoq.com/presentations/Secure-Mashups-in-ECMAScript-5 http://qconsf.com/dl/qcon-sanfran-2011/slides/MarkS.Miller_RemainingHazardsAndMitigatingPatternsOfSecureMashupsInEcmaScript5.pdf .

In particular, we need the following guarantee:

    var p = Q(something).then(callback, errback);
    var p = Q(something).send('name', ...args);
    // etc, for chains that begin with Q(something)...

where Q is the trusted Q, but 'something' is obtained from an untrusted client, must not cause any of that client's untrusted code to run during the present turn.

This becomes even more important starting in ES6 because proxies introduce far more potential interleaving hazards, raising the priority to have a way to reliably protect against these. Even if something is a proxy, the code above must not trigger any of its traps during the current turn. That's why the code at https://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/ses/makeQ.js#204 only does synchronously operations that proxies don't trap. Any why the assimilation check is postponed till a later turn, because it is impossible to test a duck type without triggering a trap. (Note to @kriskowal : Last I looked, the assimilation test in your Q implementation was not safely asynchronous in this sense.)

getify commented 11 years ago

One (unsolicited) data-point about duck-typing to test (for promises or thenables or whatever): I have two async-flow-control libs (in use on a number of projects) which take a somewhat abstracted approach to promises that's more high-level task-oriented than generic promise utility.

One is asyncGate and the other is asyncSteps... the main difference being whether the API processes in "parallel" or "serial". Both libs have a then() method on them, because "then" makes semantic sense for these tasks, even though my libs are not in any way intended to be Promise/A+ (or any other standard) compatible. You cannot assume someone uses then() because they are trying to conform to Promises/A+. Some of us (ok, maybe only me) don't like it and don't care to conform.

By some definitions of what I read in this thread, these libs would be the counter-example that just testing for a then() is a safe enough test. Then again, I highly doubt someone's going to be using my non-standard promise-related libs along side a real promise system. The purpose of my libs is to be simpler and more accessible than real promises, so you'd almost certainly use one or the other, and would be blame-ably crazy for trying to mix the two.

sicking commented 11 years ago

Another solution might be to allow pages to provide a callback which tests if a given object is a Future. So something like:

Future.addIsFutureHook(function(v) {
  return !!v && typeof v.then === "function";
}

This way the Promise/A+ library or any other library can install their own hooks and test as conservatively as they can. Multiple hooks would be allowed and if any of them claim that a value is a Future, then it's considered a Future.

Over time hopefully pages will migrate to using just built-in promises and the hook ends up unused and could potentially even be removed.

erights commented 11 years ago

@sicking Do you mean, to test if it is a future/promise, or to test if it is a thenable? What is the purpose of the test?

sicking commented 11 years ago

As far as I can tell, what we're mostly concerned with here is to test if something is thenable. But since the subject of the thread is "is-a Future" I used that terminology. But "thenable" is more clear.

Generally speaking I think providing a function like Future.isThenable() is very valuable if we can make it return true for Promise/A+ promises etc. The implementation of which is the subject of this thread. Using a Future.addIsThenableHook might be one option.

It might additionally be useful to provide a Future.isFuture() function which would only return true if the value is object which implements the full Future interface, including the state, value and error properties.

sicking commented 11 years ago

The purpose of a "thenable" test would be to allow people to write APIs which accept values either synchronously by passing the value directly, or asynchronously by passing a thenable object. The Cursor proposal that I made quickly turned out to need this ability, and I would expect other places to need this too once thenables get used more consistently across the platform.

erights commented 11 years ago

@sicking Why make the thenable test configurable? Since an assimilation convention makes the thenable test a coordination point among separately developed promise/future libraries, aren't we all better off if we jointly agree on a simple adequate convention? Given than a thenable test must be duck anyway, and thus must imply all the hazards @dherman raises, I think the test you wrote

!!v && typeof v.then === "function"

is the one we should settle on. Anything more "precise" only adds complexity without significantly reducing the hazard.

Worse, an extensible thenable test, where each library adds their own extension to Future.addIsFutureHook, would mean that library A, even in the absence of malice, might extend the thenable test so as to break a working relationship between libraries B and C.

sicking commented 11 years ago

The point of a configurable testable test is to avoid exactly the type of breakage you are talking about. While I suspect that !!v && typeof v.then === "function" is the best we can do for any hard coded test, I would expect that any hook provided to addIsThenableHook would be at least that strict.

The whole point of the isThenable test is to check if you can call .then(callback) on the value and expect to get called back. So if a hook doesn't even check that there's a .then function then that hook is dramatically broken which would likely be discovered sooner rather than later.

So I don't think that library A would be able to break interactions between B and C any more than the simple typeof test would.

erights commented 11 years ago

Library B and C might count on D being classified as thenable and count on E being classified as not thenable. If library A changes either classification, then it can break the B-C relationship.

On Fri, Mar 8, 2013 at 12:23 PM, Jonas Sicking notifications@github.comwrote:

The point of a configurable testable test is to avoid exactly the type of breakage you are talking about. While I suspect that !!v && typeof v.then === "function" is the best we can do for any hard coded test, I would expect that any hook provided to addIsThenableHook would be at least that strict.

The whole point of the isThenable test is to check if you can call .then(callback) on the value and expect to get called back. So if a hook doesn't even check that there's a .then function then that hook is dramatically broken which would likely be discovered sooner rather than later.

So I don't think that library A would be able to break interactions between B and C any more than the simple typeof test would.

— Reply to this email directly or view it on GitHubhttps://github.com/slightlyoff/DOMFuture/issues/41#issuecomment-14643485 .

Text by me above is hereby placed in the public domain

Cheers, --MarkM

sicking commented 11 years ago

Please provide a more concrete example.

I'm also not convinced that what you are describing is more likely than that someone creates an object with a function named 'then' but which is not future/promise related at all. That is the big concern with the alternative approach.

sicking commented 11 years ago

To put it another way, the risk seems higher that !!v && typeof v.then === "function" would classify E as thenable when it isn't. Than that the test provided by library A would classify E as thenable when it isn't.

This because !!v && typeof v.then === "function" is an extremely weak test and I would expect any library to provide a test at least as strong.

erights commented 11 years ago

library B:

var D = {then: function(callback, errback) {...}};
var E = {when: function() {... something unrelated ...}};

library C:

var DP = Q(D);
var EP = Q(E);

library A:

Future.addIsFutureHook(function(v) {
  return !!v && typeof v.when === "function";
});

If you don't like this example because the added hook function isn't realistic, please show an example of a hook function you do think is realistic and I'll show you a program in this pattern broken by it.

sicking commented 11 years ago

That hook is very broken. You should only claim that an object is "thenable" if it has a then function which takes two optional arguments. Once which is called upon successful creation of a result, and one that is called on error.

If an object is not implementing that contract, then you should not claim that it's thenable. The purpose of the thenable test is to enable people to write code like:

x = someLibrary.doStuff();
if (Future.isThenable(x)) {
  x.then(processResult);
}
else {
  processResult(x);
}

This code would obviously not work if x had a when function instead of a then function. Likewise it wouldn't work if x had a then function which did something unrelated to promises.

This pattern is needed both by the internals of the Future code, and will surely be needed in many other places. See my cursor proposal for example.

Two examples of realistic hooks:

Future.addIsThenableHook(function(v) {
  return !!v && typeof v.then === "function" && v.then.isapluspromise === true;
});

Future.addIsThenableHook(function(v) {
  return !!v && typeof v.then === "function" &&
         typeof v.catch === "function" &&
         v.myhomegrownpromiselibrary;
});
erights commented 11 years ago

After A executes all these addIsThenableHook calls, is an object considered thenable iff it passes all of them or iff it passes any of them? I'm still puzzled about what you're proposing.

If "all", then D would no longer be considered thenable, breaking the B-C relationship. If "any", then these calls have no effect, since they're all more specific than the original thenable test already registered, say, by B and C.

sicking commented 11 years ago

it's thenable if it passes "any" of the tests.

I'm proposing that the only "preinstalled" test is one that only returns true for objects created using new Future (as well as created though built-in APIs like revised IndexedDB API etc).

What test are you envisioning that B and C would register that would make this system not work?

The intent is that a revised version of Promises/A+ would set some identifiable flag on all promises that it creates and then register using the hook to classify all its promises as to make them compatible with the Future API.

domenic commented 11 years ago

The more likely outcome of this, IMO, is that people keep using their Promises/A+ library and whenever they see a DOMFuture, they wrap it in order to get the expected behavior, just like they do with jQuery promises today.

On Mar 9, 2013, at 0:31, "Jonas Sicking" notifications@github.com<mailto:notifications@github.com> wrote:

it's thenable if it passes "any" of the tests.

I'm proposing that the only "preinstalled" test is one that only returns true for objects created using new Future (as well as created though built-in APIs like revised IndexedDB API etc).

What test are you envisioning that B and C would register that would make this system not work?

The intent is that a revised version of Promises/A+ would set some identifiable flag on all promises that it creates and then register using the hook to classify all its promises as to make them compatible with the Future API.

— Reply to this email directly or view it on GitHubhttps://github.com/slightlyoff/DOMFuture/issues/41#issuecomment-14658429.

erights commented 11 years ago

@sicking I am assuming that B and C had already registered the test

Future.addIsFutureHook(function(v) {
  return !!v && typeof v.then === "function";
}

as that is the generally accepted convention. If so, then the additional tests registered by A have no effect.

erights commented 11 years ago

@sicking Very specifically for this example, C would register this conventional hook, in order to recognize objects such as B's D object above as a thenable. Neither of your more specific hooks by themselves would have been adequate to recognize B's D as thenable.

sicking commented 11 years ago

Mark: I feel like we are going in circles.

The goals that I have are:

  1. It would be preferrable if the Future API was compatible with existing promise libraries in so far as if you call
new Future(function(r) {
  var x = functionReturningCustomPromise();
  r.resolve(x);
})

or

functionReturningFuture().then(function(v) {
  return functionReturningCustomPromise(v);
}).then(function(v) {
  console.log(v);
});

It would further be great if people wanting to integrate with code which uses promises or Futures could write code like:

var x = doSomething();
if (<some function testing if an object is "thenable">(x)) {
  x.then(myHandler);
}
else {
  myHandler(x);
}
  1. I'd like to see Futures standardized and made part of the web platform.
  2. I'd like to avoid building the name then into the ECMAScript language. I.e. I'd like to avoid another scenario like __proto__ where a property name has a magic meaning on all objects.

My proposal for resolving 1-3 was to add the following two functions:

Future.isThenable = function(x) {
  if(!x) {
    return false;
  }
  if (x.[[Class]] === "Future") {
    return true;
  }
  return @callbacks.some(function(callback) {
    try { return callback(x); }
    return false;
  });
};
Future.addIsThenableHook(x) {
  @callbacks.push(x);  
}

This would surely result in people adding completely malfunctional hooks. It's also the case that people would add hooks that simply test typeof x.then === "function". But I strongly prefer to have those bugs and suboptimal behaviors in libraries and only affecting people using those libraries, than built into the web platform and affecting everyone.

If we add a test like !!v && typeof v.then === "function"; into the Future API, either as part of a Future.isThenable function, or built into the ResolverCallbacks.resolve function, then we'll either fail 3 or have to give up on 2.

Adding a isThenable function paired with a addIsThenableHook is only one way to resolve 1-3. If you have other proposals, then I'm all ears. But it'd at least be nice if people understood the problem statement.

If you simply don't care about one of the points above, that's certainly a valid answer, just one I don't agree with.

erights commented 11 years ago

@sicking Hi Jonas, well put. But I think a configurable thenable test will in practice create all the same problems you're trying to avoid, because some popular libraries will add broad tests. And configurability will create more problems, because these additional tests will disrupt the assumptions of other co-loaded libraries.

So, in light of this, I'm willing to give up on 3 because I believe that a configurable thenable test will sacrifice it anyway. And since it will be gone either way, best to codify one conventional way.

sicking commented 11 years ago

I'd personally give up 1 rather than give up 3. Like @domenic says, people can always manually convert between different promises.