Promise returned by DOMRequest does not keep the mozSetting lock alive

[Tim Chien]

Hi,

Ever since bug 839838 we are offered a DOMRequest#then method, which
is great. However, it's worthy to point out when the then() callback
got executed, the mozSetting lock will be closed already. See the
following code:

1)

var lock = navigator.mozSettings.createLock();
lock.get('foo').then(function(value) {
var num = value.foo || 0;
num++;
console.log(num);
return lock.set({foo: num});
}).catch((e) => { console.error(e); })

2)

var lock = navigator.mozSettings.createLock();
lock.get('foo').onsuccess = function(evt) {
var num = evt.target.result.foo || 0;
num++;
console.log(num);
lock.set({foo: num});
};


The (2) will correctly use the foo key as a counter and (1) does not.

I realized this is something we inherited from IndexedDB, as pointed
out by [1]. However I would love to throw the discussion here and see
if it's possible to make improvement, maybe by implement a sync
thenable for that then method?

[1] http://pouchdb.com/2014/10/26/10-things-i-learned-from-reading-and-writing-the-pouchdb-source.html
point 7.

--
Tim Guan-tin Chien, Engineering Manager and Front-end Lead, Firefox
OS, Mozilla Corp. (Taiwan)

[Jonas Sicking]

On Tue, Nov 18, 2014 at 11:03 PM, Tim Chien <timd...@mozilla.com> wrote:
> However I would love to throw the discussion here and see
> if it's possible to make improvement, maybe by implement a sync
> thenable for that then method?

Yeah. I think we need some internal function which allows resolving a
promise and having all registered .then callbacks be synchronously
called.

/ Jonas

[Boris Zbarsky]

On 11/25/14, 1:00 AM, Jonas Sicking wrote:
> Yeah. I think we need some internal function which allows resolving a
> promise and having all registered .then callbacks be synchronously
> called.

Wait, why?

This seems like something that would make promises a lot harder to
reason about...

-Boris

[Tim Chien]

I would imagine a Promise interface that runs synchronously can be
useful in many use cases other than this (for example, wrapping
IndexedDB IDBRequests in web content), but I do acknowledge we should
stop calling such interface "Promise".

To speak specifically about DOMRequest#then, by looking at the patch
in bug 839838, I think what I need is achievable without out touching
Promise.jsm; we can simply keep the callbacks passed to
DOMRequest#then ourselves and call them directly, instead of chain
them to the internal promise.

[Gabriele Svelto]

On 25/11/2014 15:38, Tim Chien wrote:
> I would imagine a Promise interface that runs synchronously can be
> useful in many use cases other than this (for example, wrapping
> IndexedDB IDBRequests in web content), but I do acknowledge we should
> stop calling such interface "Promise".

Just my 2p: wouldn't something like task.js be enough to make that code
act as if it were synchronous?

Gabriele

[Reuben Morais]

Not really: task.js still waits for the Promise to be resolved. SettingsLock forces the success callback to run synchronously for lock operations to work in the callback. It does:

this.open = true;
Services.DOMRequest.fireSuccess(request, result);
this.open = false;

The way around it is with a synchronous Promise-like object, or maybe by changing SettingsLock so that it’s only closed when the Promise is fulfilled? I don’t know how feasible that is… It’s not doable with the current setup where the Promise is hidden inside the DOMRequest.

-- reuben

[James Burke]

On Tue, Nov 25, 2014 at 6:38 AM, Tim Chien <timd...@mozilla.com> wrote:

> On Tue, Nov 25, 2014 at 10:09 PM, Boris Zbarsky <bzba...@mit.edu> wrote:
> I would imagine a Promise interface that runs synchronously can be
> useful in many use cases other than this (for example, wrapping
> IndexedDB IDBRequests in web content), but I do acknowledge we should
> stop calling such interface "Promise".
>

> To speak specifically about DOMRequest#then, by looking at the patch
> in bug 839838, I think what I need is achievable without out touching
> Promise.jsm; we can simply keep the callbacks passed to
> DOMRequest#then ourselves and call them directly, instead of chain
> them to the internal promise.

Some more clarification on the above:

* If DOMRequest manages the functions itself, it should still use an
async trigger for calling the callbacks, to maintain Promise
expectations.

* If the .then on the DOMRequest is called later, after initial
completion of the underlying request, then if there are resources or
locks needed it would need to re-acquire them. Not sure that is
possible given the original post’s code where the lock is acquired
outside the object that has the then() capability.

In general, I think your suggestion of ‘“stop calling such an
interface “Promise”' is the best choice.

In this case, it just sounds like DOMRequest is not a good fit for
promise use, based on the APIs that use them and their assumptions on
releasing resources? If so, it is best to just remove the .then
capability from DOMRequest, and document that this is why DOMRequest
exists as a separate construct to promises.

That, or the mozSettings API might need a different API approach. Does
not help the IDB situation though.

James

[Tim Chien]

On Wed, Nov 26, 2014 at 1:59 AM, James Burke <jrb...@gmail.com> wrote:
> On Tue, Nov 25, 2014 at 6:38 AM, Tim Chien <timd...@mozilla.com> wrote:
>> On Tue, Nov 25, 2014 at 10:09 PM, Boris Zbarsky <bzba...@mit.edu> wrote:
>> I would imagine a Promise interface that runs synchronously can be
>> useful in many use cases other than this (for example, wrapping
>> IndexedDB IDBRequests in web content), but I do acknowledge we should
>> stop calling such interface "Promise".
>>
>> To speak specifically about DOMRequest#then, by looking at the patch
>> in bug 839838, I think what I need is achievable without out touching
>> Promise.jsm; we can simply keep the callbacks passed to
>> DOMRequest#then ourselves and call them directly, instead of chain
>> them to the internal promise.
>
> Some more clarification on the above:
>
> * If DOMRequest manages the functions itself, it should still use an
> async trigger for calling the callbacks, to maintain Promise
> expectations.
>

I didn't know "Promise expectations" means the then() callbacks must
be called async. But even so we can still keep the method confirm to
Promise expectations by doing the following:

1. If the then() is called before the DOMRequest resolves/rejects,
call the onFulfill/onReject callbacks at the same loop of
onsuccess/onerror events.
2. If the then() is called after the DOMRequest resolves/rejects, call
the onFulfill/onReject callbacks asynchronously.

Both 1 and 2 still call the callback asynchronously and make the lock
survive long enough for (1)*, but not (2).

* (1) is only useful for simple use case like mentioned in the thread,
as soon as people do chain callbacks with normal promises like
Promise.resolve(req).catch(callback) or Promise.all([req,
req2]).then(callback) the callbacks will not get a open lock.

> * If the .then on the DOMRequest is called later, after initial
> completion of the underlying request, then if there are resources or
> locks needed it would need to re-acquire them. Not sure that is
> possible given the original post’s code where the lock is acquired
> outside the object that has the then() capability.
>

Exactly.

> In general, I think your suggestion of ‘“stop calling such an
> interface “Promise”' is the best choice.
>
> In this case, it just sounds like DOMRequest is not a good fit for
> promise use, based on the APIs that use them and their assumptions on
> releasing resources? If so, it is best to just remove the .then
> capability from DOMRequest, and document that this is why DOMRequest
> exists as a separate construct to promises.
>

Since DOMRequest is being used beyond IDB-related stuff, so then()
will still be useful even if it can't fulfill the use case in this
thread. We could just accept that as a defect without removing it.

> That, or the mozSettings API might need a different API approach. Does
> not help the IDB situation though.
>
> James

[Jonas Sicking]

On Tue, Nov 25, 2014 at 6:09 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
> On 11/25/14, 1:00 AM, Jonas Sicking wrote:
>>
>> Yeah. I think we need some internal function which allows resolving a
>> promise and having all registered .then callbacks be synchronously
>> called.
>
> Wait, why?
>
> This seems like something that would make promises a lot harder to reason
> about...

Note that the callbacks will still always be fired asynchronously from
the webpage's point of view. For the same reason that we always fire
onsuccess and onerror events on DOMRequests asynchronously currently.

The reason we need this is to handle APIs that use auto-closing locks.
See a more detailed discussion here:

http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0217.html

(Back then "Promise" was called "Future")

/ Jonas

[Martin Thomson]

On 26/11/14 00:27, Jonas Sicking wrote:
> The reason we need this is to handle APIs that use auto-closing locks.
> See a more detailed discussion here:
>
> http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0217.html

We could take advantage of a feature like this for WebRTC as well.

That said, I would prefer to see a different approach taken for this,
for the reason bz noted, and because it encourages the creation of a
usage model that has nasty side-effects.

You could implement this by adding a shim on .then() to do something like:

let lock = Lock.acquire();
let lockCount = 0;
let p = doTheRealWork();
let origThen = p.then;
let release = x => { --lockCount || lock.release(); return x; }
p.then = (cb, ecb) => {
++lockCount;
return origThen.call(p, cb, ecb)
.then(release,
e => { release(); throw e; });
}
// overwrite .catch() ?
return p;

That makes my hair stand on end though. I'm not even sure that I've
covered the corners properly, particularly around error handling. And
you only get one layer of wrapping for then(). It's a little disgusting.

[Martin Thomson]

On 2014-12-10, at 11:22, Jan-Ivar Bruaroey <j...@mozilla.com> wrote:
>
> If the callback must be executed inside an auto-closing lock, then why not pass the callback in?

This might be the key realization. It’s not like every callback has to be replaced with a Promise :)

[Jan-Ivar Bruaroey]

On 12/8/14, 6:49 PM, Martin Thomson wrote:
> That makes my hair stand on end though. I'm not even sure that I've
> covered the corners properly, particularly around error handling. And
> you only get one layer of wrapping for then(). It's a little disgusting.

Quite, and it holds a lock across dispatches, defeating the safety of
auto-closing locks that Jonas linked to.

Promises work one way, callbacks another. Before we tweak (or emulate)
these primitives, we should be certain we're not misapplying them.
Specifically, why use a promise for something synchronous?

I think I'd prefer an API that was more explicit about the locking
involved: If the callback must be executed inside an auto-closing lock,

then why not pass the callback in?

var lock = navigator.mozSettings.createLock();
lock.get('foo', function(value) {

var num = value.foo || 0;
num++;
console.log(num);
return lock.set({foo: num});

}).then(function() { /* Value set. What's next? */ } )
}).catch((e) => { console.error(e); })

This could be implemented so that any exception thrown in the callback
would show up in the promise-catch.

[Jan-Ivar Bruaroey]

Just a follow-up: What I propose uses a promise AND a callback, since
callbacks alone are notorious for lousy error-handling.

The callback is an argument to the asynchronous call, which when the
operation is processed later, gets executed inside an auto-closing lock
(i.e. synchronously). Any exception in the callback causes the promise
to get rejected back to the caller of the asynchronous call.

..: Jan-Ivar :.

[Jonas Sicking]

On Wed, Dec 10, 2014 at 11:32 AM, Martin Thomson <m...@mozilla.com> wrote:
> On 2014-12-10, at 11:22, Jan-Ivar Bruaroey <j...@mozilla.com> wrote:
>>
>> If the callback must be executed inside an auto-closing lock, then why not pass the callback in?
>
> This might be the key realization. It's not like every callback has to be replaced with a Promise :)

This is indeed very true. But I think in the specific case here, the
Promise syntax is very helpful in creating readable code.

I'd actually love to try using a Promise here. This is a
FirefoxOS-only API and unlikely to ever be anything else. It's mainly
used by certified apps, but there are a few cases where we also enable
3rd party apps to use them.

So this is a prime case for getting feedback about how well it'd work
to use Promises in this situation will work, what problems that we'll
run into, and if those problems can be fixed.

If we ever want to use Promises for IndexedDB, which has been the main
ask from developers regarding IDB, we need to find answers to these
questions.

/ Jonas

[Jan-Ivar Bruaroey]

On 12/19/14, 3:57 PM, Jonas Sicking wrote:
> I'd actually love to try using a Promise here.

Jonas caught me up on irc that we need to auto-close over several
asynchronous callbacks, safely. Basically, extend the life of the lock
whenever one or more new asynchronous requests are initiated from within
the last auto-closing callback.

The open_flag is cleared too soon with promises, which seems solvable,
as others have pointed out, by returning a promise-like object under
control of the browser. I think this could be generalized as follows:

Have lock.get("key") or whatever return an AutoClosingPromise that wraps
today's promise, with an internal "ticket" to be closed.

AutoClosingPromise could be implemented something like this:
http://jsfiddle.net/jib1/w0ufvahL

AutoClosingPromise does two things:
1) Closes a ticket (clears open_flag) *after* executing success/failure.
2) If a registered fulfillment/rejection callback returns another
AutoClosingPromise, it forwards that one's ticket to the
AutoClosingPromise it returned in its own .then() function.

This should be safe because AutoClosingPromises are guaranteed to
resolve/reject, as they're issued by the browser, and tickets are
private and do not get forwarded if plain promises are jammed in-between
AutoClosingPromises.

This should mesh with regular promises and not require altering them.

..: Jan-Ivar :.

[Tim Guan-tin Chien]

Jan-Lvar,

This is a great idea! Thanks. Just to confirm what AutoClosingPromise
can and should protect, consider the following:

(3)

var lock = navigator.mozSettings.
createLock();
var p = lock.get('foo').then(function(value) {

var num = value.foo || 0;
num++;
console.log(num);

return myOtherPromise.then(() => lock.set({foo: num}));
}).catch((e) => { console.error(e); })

Since the time lock.set() is called will be depend on myOtherPromise,
and since the callback returns a normal Promise instead of an
AutoClosingPromise, lock should be closed before set() is called. Is
this correct?

(I haven't thought about the use case of this example -- maybe
|myOtherPromise| is a promise returned from a function taking |num| as
input?)

On Sun, Dec 21, 2014 at 10:27 AM, Jan-Ivar Bruaroey <j...@mozilla.com> wrote:
> On 12/19/14, 3:57 PM, Jonas Sicking wrote:

>> I'd actually love to try using a Promise here.
>

> _______________________________________________
> dev-webapi mailing list
> dev-w...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-webapi

[Jan-Ivar Bruaroey]

On 12/21/14, 5:56 AM, Tim Guan-tin Chien wrote:
> This is a great idea! Thanks. Just to confirm what AutoClosingPromise
> can and should protect, consider the following:
>
> (3)
>
> var lock = navigator.mozSettings.
> createLock();
> var p = lock.get('foo').then(function(value) {
> var num = value.foo || 0;
> num++;
> console.log(num);
> return myOtherPromise.then(() => lock.set({foo: num}));
> }).catch((e) => { console.error(e); })
>
> Since the time lock.set() is called will be depend on myOtherPromise,
> and since the callback returns a normal Promise instead of an
> AutoClosingPromise, lock should be closed before set() is called. Is
> this correct?

Correct. Regular promises in general cannot be trusted to ever complete.

I think this satisfies what Anne suggests in [1].

> (I haven't thought about the use case of this example -- maybe
> |myOtherPromise| is a promise returned from a function taking |num| as
> input?)

AutoClosingPromise would allow us to design locks able to span
participating asynchronous calls, but holding a lock over arbitrary
asynchronous calls doesn't seem safe or wise to me. Your other function
may block on a whole other sub-system for instance, inviting deadlocks
in the design.

.: Jan-Ivar :.

[1] http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0059.html

[Jan-Ivar Bruaroey]

On 12/21/14, 5:56 AM, Tim Guan-tin Chien wrote:

> This is a great idea! Thanks. Just to confirm what AutoClosingPromise
> can and should protect, consider the following:
>
> (3)
>
> var lock = navigator.mozSettings.
> createLock();
> var p = lock.get('foo').then(function(value) {
> var num = value.foo || 0;
> num++;
> console.log(num);
> return myOtherPromise.then(() => lock.set({foo: num}));
> }).catch((e) => { console.error(e); })
>
> Since the time lock.set() is called will be depend on myOtherPromise,
> and since the callback returns a normal Promise instead of an
> AutoClosingPromise, lock should be closed before set() is called. Is
> this correct?

Correct. Regular promises in general cannot be trusted to ever complete.

I think this satisfies what Anne suggests in [1].

> (I haven't thought about the use case of this example -- maybe
> |myOtherPromise| is a promise returned from a function taking |num| as
> input?)

AutoClosingPromise would allow us to design locks able to span
participating asynchronous calls, but holding a lock over arbitrary
asynchronous calls doesn't seem safe or wise to me. Your other function
may block on a whole other sub-system for instance, inviting deadlocks
in the design.

..: Jan-Ivar :.

[1] http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0059.html

[Martin Thomson]

On Sun, Dec 21, 2014 at 11:27 AM, Jan-Ivar Bruaroey <j...@mozilla.com> wrote:
>
> AutoClosingPromise would allow us to design locks able to span
> participating asynchronous calls, but holding a lock over arbitrary
> asynchronous calls doesn't seem safe or wise to me. Your other function may
> block on a whole other sub-system for instance, inviting deadlocks in the
> design.

Your design doesn't guarantee release either.

Also, in terms of details:

- It's not AutoClosingPromise, it's a LockExtendingPromise
- I'd like to see a positive affirmation from takeTicket() that the ticket
was accepted (a return value would be good)
- You could give all promises that chain off this new promise duck-type a
chance to accept the ticket; I think you should (fcfs, of course)

[Jan-Ivar Bruaroey]

On 12/22/14, 12:32 PM, Martin Thomson wrote:
> On Sun, Dec 21, 2014 at 11:27 AM, Jan-Ivar Bruaroey <j...@mozilla.com> wrote:
> Your design doesn't guarantee release either.

I think it does guarantee release, given a few assumptions.

1. AutoClosingPromise is a browser interface.

2. Only browser-methods can instantiate AutoClosingPromise.

3. A browser-method implicitly guarantees calling resolve or reject on
all AutoClosingPromises it returns.

4. We don't forward tickets to regular promises.

The ticket-forwarding is internal to AutoClosingPromise, so at no point
can a ticket block on fulfillment from something other than the browser.

If I'm missing something, please point me to it.

> Also, in terms of details:
>
> - It's not AutoClosingPromise, it's a LockExtendingPromise

I see extending a lock as one application. There's no mention of lock.

> - I'd like to see a positive affirmation from takeTicket() that the ticket
> was accepted (a return value would be good)
> - You could give all promises that chain off this new promise duck-type a
> chance to accept the ticket; I think you should (fcfs, of course)

That would break (4).

..: Jan-Ivar :.

[Jan-Ivar Bruaroey]

On 12/22/14, 1:06 PM, Jan-Ivar Bruaroey wrote:
> The ticket-forwarding is internal to AutoClosingPromise, so at no point
> can a ticket block on fulfillment from something other than the browser.

Of course content can indirectly postpone release by requesting work
through the browser-interface in perpetuity, but you can do the same in
IndexedDB now.

..: Jan-Ivar :.