promises-aplus / promises-spec

An open standard for sound, interoperable JavaScript promises—by implementers, for implementers.
https://promisesaplus.com/
Creative Commons Zero v1.0 Universal
1.84k stars 164 forks source link

Support for IndexedDB #45

Closed awwx closed 11 years ago

awwx commented 11 years ago

This is an inquiry into whether it might be possible for Promises/A+ to support IndexedDB.

Naturally one answer might be, no, it's simply out of scope for Promises/A+.

On the other hand... it's looking like IndexedDB is going to be the major local store for web apps; the older Web SQL Database is deprecated (and never was implemented in Firefox), and it is impossible to use the venerable Web Storage safely from multiple windows (as Chrome doesn't implement the ill-specified storage mutex).

A promises interface to IndexedDB is much more pleasant than the native event model, so it would be nice to support it if we could.

The code below illustrates a scenario which uses four components. All components are written with the assumption that Promises/A+ is the only API available for promises (we don't use extensions such as otherwise and so on).

In this example the code works when the synchronous when implementation is plugged in, but fails when the asynchronous avow library is plugged in. With IndexedDB, a transaction is only kept alive if it is referenced at least once through the event loop, and so calling callbacks in the next event loop causes the transaction to be lost.

Note that merely leaving unspecified whether callbacks are synchronous or asynchronous doesn't help. To be able to keep the transaction alive, the callback must be synchronous. Thus while the IndexedDB library would presumably decide to be synchronous, the user could not reliably combine the IndexedDB library with the independent utility library merely on the strength of the Promises/A+ spec (if the spec left the synchronous behavior unspecified).

Now this is entirely speculative, and I haven't written any code or thought it through, but suppose callbacks were by default required to be asynchronous, but (for a particular execution extent) callbacks could be required to be synchronous. A library would either generate a synchronous or asynchronous callback depending on the mode, or, if it could only support asynchronous callbacks (xhr requests etc.) it would throw an error if it were called in the wrong mode. The user could say however for a particular callback "no, this one can be asynchronous" (because it didn't use the transaction or whatever).

Well, probably too complicated to make it into Promises/A+, but interesting to think about anyway.

require ['avow', 'when'], (avow, _when) ->

  ## base Promises implementation

  pending = ->
    deferred = _when.defer()
    {
      promise: deferred.promise
      fulfill: deferred.resolve
      reject:  deferred.reject
    }

  # or

  # pending = ->
  #   v = avow()
  #   {
  #     promise: v.promise
  #     fulfill: v.fulfill
  #     reject: v.reject
  #   }

  ## a promises utility library

  fulfilled = (val) ->
    pend = pending()
    pend.fulfill(val)
    pend.promise

  isPromise = (x) -> x and typeof(x.then) is 'function'

  toPromise = (x) ->
    if isPromise(x)
      x
    else
      pend = pending()
      pend.fulfill(x)
      pend.promise

  trace = (name, promiseOrValue) ->
    promise = toPromise(promiseOrValue)
    promise.then((value) -> console.log 'trace', name, value)
    promise

  ## a Promises based wrapper library for indexDB

  indexedDB = window.indexedDB or
              window.mozIndexedDB or
              window.webkitIndexedDB

  request_to_promise = (req) ->
    pend = pending()
    req.onerror   = (event) -> pend.reject()
    req.onsuccess = (event) -> pend.fulfill(req.result)
    pend.promise

  delete_database = (database_name) ->
    request_to_promise(indexedDB.deleteDatabase(database_name))

  open_database = (database_name, version, onUpgradeNeeded) ->
    req = indexedDB.open(database_name, version)
    req.onupgradeneeded = (event) ->
      onUpgradeNeeded(event.target.result)
    request_to_promise req

  db_transaction = (db, store_names) ->
    fulfilled(db.transaction(store_names))

  store_get = (objectStore, key) ->
    request_to_promise objectStore.get(key)

  ## user code

  setup_database = (db) ->
    objectStore = db.createObjectStore("kv", { keyPath: "k" })
    objectStore.add({k: 'a', v: 1})
    objectStore.add({k: 'b', v: 2})

  delete_database('test')
  .then(=> open_database('test', 1, setup_database))
  .then((db) -> db_transaction(db, ['kv']))
  .then((tx) ->
     trace('transaction mode', tx.mode)
     .then(-> trace('a', store_get(tx.objectStore('kv'), 'a')))
     .then(-> trace('b', store_get(tx.objectStore('kv'), 'b')))
  )
  .then(null, (reason) =>
    console.log 'error', reason, reason.message
  )
domenic commented 11 years ago

First of all, CoffeeScript in a JavaScript repo is disrespectful :-/.

Second, I think I am fundamentally misunderstanding the problem here. IndexedDB uses callbacks specifically because it is asynchronous in nature. I see no possible way for a database retrieval to return in the same turn of the event loop as it was issued. So how could callbacks ever be synchronous in the first place?

Note that there is no requirement that you wait a tick after receiving a value from a callback to resolve your promise. There is simply the requirement that the promise handlers not be called in the same tick they were added to the promise. You can "synchronously" deliver information from an IndexedDB callback to the promise handlers as long as those handlers were attached in a past tick---which, as I said above, seems to be the only possible situation you could ever find yourself in.

Third, that misunderstanding aside, this whole "you can't wait a tick" problem you describe is similar to that faced by Node.js streams. The solution there was to be careful; streams and promises still interact fine.

awwx commented 11 years ago

First of all, CoffeeScript in a JavaScript repo is disrespectful :-/.

Oops, sorry about that!

Third, that misunderstanding aside, this whole "you can't wait a tick" problem you describe is similar to that faced by Node.js streams.

I use Node, but I'm not familiar with the Node.js stream issue.

Second, I think I am fundamentally misunderstanding the problem here. IndexedDB uses callbacks specifically because it is asynchronous in nature. I see no possible way for a database retrieval to return in the same turn of the event loop as it was issued. So how could callbacks ever be synchronous in the first place?

Ah, the point is not that you couldn't carefully use an IndexedDB library by itself, and get it to work. Like you say, the IndexedDB callback is asynchronous.

But, if you want to build upon an IndexedDB library using a promises coding style, you'd have to use noncompliant promises.

Normally the hope is that if everybody follows the spec then libraries will interoperate; but here the program breaks if you wish to use a Promises/A+ compliant utility library with a Promises/A+ compliant IndexedDB library.

Which, like I said, might simply be out of scope for Promises/A+. But, given the apparent prominence of IndexedDB, I thought it might be worthwhile to think about whether there might be a possible solution... even if it's a long shot.

domenic commented 11 years ago

But, if you want to build upon an IndexedDB library using a promises coding style, you'd have to use noncompliant promises.

I guess I'd need to see some (JavaScript) example of this and how it fails to really understand what you're trying to say.

awwx commented 11 years ago

I guess I'd need to see some (JavaScript) example of this and how it fails to really understand what you're trying to say.

OK!

In this scenario trace is an example of a Promises utility built upon Promises/A+. (It is passed a promise, and it simply prints to the console a log of the promise's value when it is fulfilled).

The user program reads some values from the database and uses trace to print them out. It illustrates the issue that trace by itself is Promises/A+ compliant, and the IndexedDB library is Promises/A+ compliant, but when the user program uses them together in a reasonable way then it is broken by IndexedDB closing the "inactive" transaction.

The program works when it uses the synchronous when library but breaks with avow.

When run it prints out

trace transaction mode readonly
trace a Object {k: "a", v: 1}
trace b Object {k: "b", v: 2} 
<!doctype html>
<html>
<head>
<script data-main="jsdb" src="require.js"></script>
</head>
<body>
</body>
</html>
require(['avow', 'when'], function(avow, _when) {

  /* choose a Promises implementation */

  var pending = function() {
      var deferred = _when.defer();
      return {
        promise: deferred.promise,
        fulfill: deferred.resolve,
        reject:  deferred.reject
      };
  };

  /* or...
  var pending = function() {
      var v = avow()
      return {
        promise: v.promise,
        fulfill: v.fulfill,
        reject:  v.reject
      }
  };
  */

  /* ******
     a Promises utility library
  */

  var fulfilled = function(val) {
      var pend = pending();
      pend.fulfill(val);
      return pend.promise;
  };

  var isPromise = function(x) {
      return x && typeof x.then === 'function';
  };

  var toPromise = function(x) {
      if (isPromise(x)) {
          return x;
      } else {
          var pend = pending();
          pend.fulfill(x);
          return pend.promise;
      }
  };

  var trace = function(name, promiseOrValue) {
      var promise = toPromise(promiseOrValue);
      promise.then(function(value) {
          console.log('trace', name, value);
      });
      return promise;
  };

  /* *****
     an IndexedDB library that provides a Promises interface
  */

  var indexedDB = window.indexedDB || window.mozIndexedDB || window.webkitIndexedDB;

  var request_to_promise = function(req) {
      var pend = pending();
      req.onerror = function(event) {
          pend.reject();
      };
      req.onsuccess = function(event) {
          pend.fulfill(req.result);
      };
      return pend.promise;
  };

  var delete_database = function(database_name) {
      return request_to_promise(indexedDB.deleteDatabase(database_name));
  };

  var open_database = function(database_name, version, onUpgradeNeeded) {
      var req = indexedDB.open(database_name, version);
      req.onupgradeneeded = function(event) {
          onUpgradeNeeded(event.target.result);
      };
      return request_to_promise(req);
  };

  var db_transaction = function(db, store_names) {
      return fulfilled(db.transaction(store_names));
  };

  var store_get = function(objectStore, key) {
      return request_to_promise(objectStore.get(key));
  };

  /* *****
     user program
  */

  var setup_database = function(db) {
      var objectStore = db.createObjectStore("kv", {keyPath: "k"});
      objectStore.add({k: 'a', v: 1});
      objectStore.add({k: 'b', v: 2});
  };

  delete_database('test').then(function() {
      return open_database('test', 1, setup_database);
  }).then(function(db) {
      return db_transaction(db, ['kv']);
  }).then(function(tx) {
      return trace('transaction mode', tx.mode).then(function() {
          return trace('a', store_get(tx.objectStore('kv'), 'a'));
      }).then(function() {
          return trace('b', store_get(tx.objectStore('kv'), 'b'));
      });
  }).then(null, function(reason) {
      console.log('error', reason, reason.message);
  });
});
juandopazo commented 11 years ago

I don't see the issue either. This is probably how a promises implementation would force the flow of a request to be async: http://jsbin.com/olikim/1/edit (first time I try out indexedDB, copypaste from MDN).

Can you simplify your example so that it shows how an asynchronous callback breaks an indexedDB query?

awwx commented 11 years ago

http://jsbin.com/olikim/1/edit

One timeout is OK. If you have two in a row (without doing something more with the transaction) then the transaction will be closed on you for being "inactive".

Can you simplify your example so that it shows how an asynchronous callback breaks an indexedDB query?

All you need to do is have an extra then. (Which triggers a second asynchronous callback, which causes the transaction to close). But if the example merely had extra then's, you say "why do that?" :) ...which is why the purpose of the example is to show how the problem makes Promise/A+ libraries non-composable.

juandopazo commented 11 years ago

Like this? http://jsbin.com/olikim/3/edit

awwx commented 11 years ago

Like this? http://jsbin.com/olikim/3/edit

You need to move your setTimeout's between the point that you get the transaction and when you use it. (Your first timeout is before you get the transaction in question, and your second two are after the database value has already been retrieved, so the transaction isn't being used at that point).

briancavalier commented 11 years ago

I must be too tired to fully understand the examples here :) I'll look at them with fresh eyes tomorrow.

juandopazo commented 11 years ago

So basically you mean...

var transaction = db.transaction(["foo"]);
setTimeout(function () {
  // do something with the transaction
  // but the transaction has expired. BOOM!
}, 100);

I don't see how this has to do with promises. This seems to be an issue with how you would write your promise-based abstraction layer. It looks like something you should be hiding from the user.

IMHO issues like this will always exist, workarounds will always have to be written, so promises should be spec'd thinking about the convenience of library users, not library authors.

In this case you should think about how to provide your users with something like this that returns a promise:

var promise = db('foo', 2).store('bar').get('someId');
promise.then(function (value) {
  // here's the value
}, function (err) {
  // the query failed!
});
awwx commented 11 years ago

so promises should be spec'd thinking about the convenience of library users, not library authors.

That's right: I'm concerned with the library user, not the library author.

I write a IndexedDB library. Someone else writes a library that implements trace or map or join. The user writes code that uses both libraries and their program breaks.

I don't care because I'll just configure when to be synchronous and write non-Promises/A+ compliant promise based code on top of my IndexedDB library that works. But that doesn't help the user who wishes that they could use interoperatable Promises/A+ libraries with IndexedDB.

ForbesLindesay commented 11 years ago

The problem is that what you're suggesting won't work. If I assume that things will always be 'synchronous' I might write:

var transaction = getTransaction();
transaction.store('bar').then(function () { return delay(100); }).get('something')
  .then(function (value) {
  });

I might expect that to work because you said you'd make your promise implementation synchronous so it would always work, but that wouldn't work. How about:

var transaction = getTransaction();
transaction.store('bar').then(function () { return random() > 0.9 ? delay(100) : null; })
  .get('something')
  .then(function (value) {
  });

which fails 1 in 10 times (assuming random() returns a number between 0 and 1).

Incidentally, I have yet to see where in either the spec or on MDN this problem is documented? I also have yet to jsbin example that actually fails. If you can create a very short simple example that fails so I can try it out, that would help considerably.

juandopazo commented 11 years ago

I don't really know why, but this does fail: http://jsbin.com/olikim/7/edit.

I think the point is that getTransaction shouldn't return a promise (which, if you think about it, is what the browser API is doing). A promise should only be returned from get.

awwx commented 11 years ago

Ah, OK, let me see if I can provide a more coherent explanation of:

and, my latest thoughts on a different "solution" (well, non-solution).

Limitation of IndexedDB

IndexedDB automatically commits transactions that haven't been used in an event loop tick. See the paragraph starting "Now that you have a transaction you need to understand its lifetime..." in Using IndexedDB.

jsbin example

Open http://jsbin.com/upazar/1 and view the console. You'll see "TransactionInactiveError".

The culprit is the setImmediate following the reading of the value "a". When the code attempts to write "b", the transaction has already been committed.

Thus while IndexedDB is asynchronous itself, you can't do any extra asynchronous operations within a transaction or you'll lose the transaction.

Not a problem for an IndexedDB library itself

Imagine that there is a library which is a thin wrapper around IndexedDB providing a Promises interface. Rather than having to write

var request = store.get(key);
request.onsuccess = function () {
    var value = request.result;
    ...
};

the user instead can write something along the lines of

store.get(key).then(function (value) {
    ...
});

Such a library is easy to write and poses no problem as long as the user doesn't do any extra asynchronous operations within a transaction.

Sad for using promises within a transaction

But what if a user would like to write promises-based code within a transaction?

If the user adds any extra then's, or calls for example when.map, or does anything else within the transaction using promises, the transaction is lost.

Which is too bad because code in a transaction often has a natural flow to it like "get a value then log the value then transform the value then log the transformed value then write the transformed value" etc... which would be nice to be able to write in a promises style.

A different "solution"

My initial thought was maybe we could do something to make callbacks synchronous inside of a transaction (that is, callbacks not related to the transaction itself, since of course the callbacks related to actually reading or writing from/to the transaction would be asynchronous; and keeping in mind that naturally not all callbacks can by synchronous: there's nothing we can do about delay etc.)... but on reflection this is sounding way too complicated. :)

Instead, perhaps the answer is to say "Don't use Promises/A+ promises within an IndexedDB transaction, because Promises/A+ is asynchronous, and if you do extra asynchronous stuff in a transaction you'll lose it".

And instead someone could use synchronous versions of trace, map, etc. which would not be Promises/A+ promises but which could be used within a transaction.

Which, if that is the right answer, puts the whole thing out of scope of the Promises/A+ spec! ^_^

ForbesLindesay commented 11 years ago

Thanks for clarifying things. Sadly I don't think this is something that can be fixed inside promises/A+

I think the only 'solution' is, use promises/A+ but keep in mind the lifetime of your transaction will end once you wait on a promise that isn't part of the transaction. This hopefully isn't the end of the world because transactions shouldn't require any async work other than reading and writing to the database itself..

Personally I'd much prefer they built a transaction object which had an explicit .commit() and .abort() pair of methods, and threw a TransactionNeverTerminated error if the transaction was garbage collected without a commit or abort being called, that would then completely solve your problem and be much easier to reason about, but that requires talking to W3C, and can't be solved here.

awwx commented 11 years ago

@juandopazo:

I don't really know why, but this does fail: http://jsbin.com/olikim/7/edit.

You create a transaction, but then you don't use it within the event loop tick. IndexedDB automatically commits transactions that aren't used at least once within a pass through the event loop (in fact, I don't think there's a way to commit a transaction explicitly, instead you simply stop adding requests to it and when all the requests have been fulfilled the transaction commits).

If you add an "oncomplete" handler to the transaction and log when the transaction completes and when your setTimeout callback is called, I suspect you'll see that your transaction is completing first. I think that you're getting the "InvalidStateError" because by the time you call transaction.objectStore("customers") the transaction is inactive.

domenic commented 11 years ago

Yeah, I don't think this is Promises/A+ related. Sounds like it's time to close :)

It is in fact a great example of why we require asynchronous resolutions. If the resolution was sometimes-synchronous, your code would work with some already-resolved promises inside the transaction scope, but then fail if those operations were refactored to be asynchronous. With always-asynchronous resolutions, such refactoring hazards are avoided in favor of consistent results.

@ForbesLindesay

Personally I'd much prefer they built a transaction object which had an explicit .commit() and .abort() pair of methods, and threw a TransactionNeverTerminated error if the transaction was garbage collected without a commit or abort being called

The problem with this is that it exposes the underlying garbage-collection semantics of a given engine to the user, which is bad as it leads to writing implementation-dependent code.

ForbesLindesay commented 11 years ago

hmm, I should have said "an unmatchable exception". Although I guess you could have code that works fine because it's never garbage collected, even though it's never used again...

Perhaps it would be better to have them timeout after say 30 seconds (way longer than a typical event loop turn) and throw at that point (if they haven't been committed/aborted)

briancavalier commented 11 years ago

Good discussion, all. Just wanted to throw in my agreement with the result here, now that I've recovered from an exhausting weekend. I think @domenic summed up my feelings:

It is in fact a great example of why we require asynchronous resolutions. If the resolution was sometimes-synchronous, your code would work with some already-resolved promises inside the transaction scope, but then fail if those operations were refactored to be asynchronous. With always-asynchronous resolutions, such refactoring hazards are avoided in favor of consistent results.

Forcing asynchronous means there is no chance of getting into or creating a sync/async non-deterministic situation or refactoring hazard.

The IndexDB transaction boundary will be a problem for any async code within a transaction, not just Promises/A+ promises. It's an unfortunate situation, but one we can't solve here.

yathit commented 11 years ago

I just want to say nothing is special about async workflow in IndexedDB API. I have wrap around all indexeddb API in promise pattern in my repo ydn-db.

pjeby commented 11 years ago

There is actually a way to work around IndexedDB incompatibility in a promises library, even if it can't be worked around in the spec itself.

If a promises library offers an API like "eventhandler(callback)", to wrap environment-level event callbacks, then it can run queued callbacks at the conclusion of the event handler, without needing to actually defer them to a subsequent run of the event loop. An IndexedDB library would then simply need to ensure that its onsuccess, etc. handlers were wrapped in eventhandler().

Unfortunately, I don't think there's a way to do this that would work across multiple promise libraries used in the same event handler, unless its via some sort of shared global, which would then need to be part of the spec.

Equally unfortunate, I'm not sure that there's actually any other use for such an API besides IndexedDB-promises, other than that it ensures no events can change the system state in between a promise's resolution and the running of its callbacks.

(Hm. Actually, that makes reasoning about state easier, and could be considered a general win.)

briancavalier commented 11 years ago

I actually think that the spec, as written (or at least as intended), is compatible with this line of thinking. The real invariant we need to preserve is that promise callbacks cannot run in the "same" execution context (i.e. stack, "same" quoted because actually writing spec language for this is hard) as the call to then in which they were registered. Running them at the end of the current tick, rather than in the next event loop turn is perfectly acceptable. Several existing implementations do this, for example, RSVP uses MutationObserver.

The IndexDB case is likely more involved than everyone simply switching to MutationObserver. As per cujojs/when#148, it seems like IndexDB would want additional promise resolutions and callbacks that occur during that end-of-tick processing to be conflated into the same end-of-tick processing run, rather than deferred until the end of a subsequent turn.

There are other potential uses for an API that allows configurable scheduling:

  1. It allows a promise impl to be adapted more easily to other platforms, which may have their own specialized task scheduler (e.g. vert.x)
  2. It allows someone to use a synchronous scheduler if they so choose (knowing full well the hazards).

Still, this seems like a glimmer of hope :)

pjeby commented 11 years ago

Still, the main thing that's required is simply a way to say, "I'm at the end of an event handler, please drain all callback queues now, and keep draining them until nothing else gets put in them." Any further customization of scheduling is really unnecessary overkill for the use case, and IndexedDB is really a pretty specialized case.

That being said, being able to get promise libraries to share a common scheduler would certainly make such drainage possible, and spec'ing a scheduler API shouldn't be hard - you just need a way to add callbacks and a way to request draining them. (And of course the implementation of the API has to schedule the draining to happen if it's not explicitly requested.)