Add a post-microtask-job list to event loops #2310

annevk · 2017-02-01T11:41:35Z

inexorabletash · 2017-02-02T18:48:06Z

Tests over in web-platform-tests/wpt#4643

annevk · 2017-02-02T18:54:59Z

Great, @smaug---- could you review the tests and this HTML Standard change? You're probably one of the few people that can...

domenic

I have a few naming concerns.

First, I think using "job" is not great here, because people might think it has to do with ES jobs. Maybe "steps" fits better with the rest of the spec, cf. "unloading document cleanup steps".

Second, I wonder if we want to give this a generic name and make it a generic hook, or just say "clean up IndexedDB transactions" and link out to an IDB algorithm. That would ensure nobody else uses this. We would then maybe generalize in the future if someone else needs it, but that seems pretty unlikely...

domenic · 2017-02-02T18:54:56Z

source

@@ -88500,6 +88500,13 @@ dictionary <dfn>PromiseRejectionEventInit</dfn> : <span>EventInit</span> {
  source</span> used. Normally, the <span>task source</span> of a <span>microtask</span> is
  irrelevant.</p>

+  <p>Each <span>event loop</span> has a <dfn data-export="">post-microtask-job list</dfn>, which is
+  « » initially. A post-microtask job cannot run scripts or be sensitive to when it is run relative


I'd fairly strongly prefer "a list, initially empty".

I was going to do that, but one of the things that annoys me about HTML is looking up the exact syntax to reference external concepts. And looking into fixing the tooling for HTML...

It should just be <span>list</span>, right?

domenic · 2017-02-02T18:55:25Z

source

+  to other post-microtask jobs.
+
+  <p class="note">This is used by the Indexed Database API to deal with transactions. <ref
+  spec=INDEXEDDB></p>


Should we add a warning that specs should not use this if at all possible, and please open an issue to discuss expanding its scope if so?

Yes we should do that.

domenic · 2017-02-02T18:56:15Z

source

-   <li><p><i>Done</i>: For each <span>environment settings object</span> whose <span>responsible
-   event loop</span> is this <span>event loop</span>, <span>notify about rejected promises</span>
-   on that <span>environment settings object</span>.</p></li>
+   <li>


I don't think nesting is necessary here. Done is just a label you jump to; it can be attached to the single notify-about-rejected-promises step, and then execution will proceed to the post-microtask job business.

You mean modify the "notify about rejected promises" algorithm or modify the current Done step to do both things?

I mean just leave the current Done step (lines 88537-88539) as-is, then add a step after it, instead of creating a step named "done" with two substeps.

smaug----

r+ assuming the end-of-task isn't needed, and Domenic had good suggestions for naming.

smaug---- · 2017-02-03T18:27:07Z

source

+     that <span>environment settings object</span>.</p></li>
+
+     <li><p>Perform each post-microtask job in the <span>post-microtask-job list</span>.</p></li>
+    </ol>



ok, this is not following the current model in Gecko's Promise handling, but I guess this is closer to Blink, right?. And that is fine I think.
In Gecko the callback is called after each Promise[1], not after handling all the Promises.
But as I said in w3c/IndexedDB#87 (comment) that feel a bit odd.

[1] http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/dom/promise/Promise.cpp#550,561-562

In Gecko the callback is also called around end-of-task, before any microtasks.
http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/xpcom/base/CycleCollectedJSContext.cpp#1400,1408
Is that needed by IDB? Can it have transactions open even outside microtasks and it would need to close those before running other microtask callbacks?
I hope that isn't needed, @inexorabletash ?

As noted in w3c/IndexedDB#87 Blink and WebKit have a microtask checkpoint before the magic that deactivates transactions. (I think that would map in your cited code to moving the ProcessMetastableStateQueue call after "step 4.1", but there's probably more subtlety.)

What's the behavior in Gecko when a microtask creates a transaction? i.e. if ProcessMetastableStateQueue is responsible for closing new stransactions, what if one is opened during PerformMicroTaskCheckpoint? When does that transaction get closed?

Can it have transactions open even outside microtasks and it would need to close those before running other microtask callbacks?

I think that's answered by the discussion in w3c/IndexedDB#87 which means I also think I'm misunderstanding the question. :( In Blink and WebKit, newly created transactions are active from the db.transaction() call until after the task completes and a microtask checkpoint occurs. Previously created transactions are active from just before event dispatch until just after, which implicitly includes all of the event handlers/listeners and microtask checkpoints induced by that algorithm.

There should be no other time a transaction is active, e.g. where a timer's task could run and find a transaction is active. Similarly, I believe it's entirely deterministic whether a microtask gets handled in a particular task's subsequent microtask checkpoint or not; that is, there shouldn't be a case where a random task's microtask (e.g. a fetch's completion) is processed during an unrelated checkpoint (e.g. one where an unrelated transaction is active). But here we're hitting the limits of my knowledge so corrections/refinement appreciated.

I think I'm asking can IDB have transactions open outside a microtask? Can something create/open a transaction when a script isn't running?

The only case where a transaction is not created by db.transaction() is when the upgradeneeded event is being dispatched. In that case the transaction starts off inactive, and is set active as part of the event dispatch, inactive when the dispatch is complete.

annevk · 2017-02-07T12:18:22Z

@inexorabletash if instead of "Perform each post-microtask job in the post-microtask-job list" we had "Cleanup Indexed Database transactions" which would be a term that the Indexed Database API defines, could that work well as a setup? That way this is not overly generic and cannot really be used by anyone else.

inexorabletash · 2017-02-07T15:51:12Z

We're okay with circular dependencies between specs I guess.

IDB would still need to associate the "jobs" with the event loop in some way without monkey patching. Would defining that as a map event loop → list of transaction cleanup jobs in IDB be appropriate? And the "Cleanup Indexed Database transactions" would "pass" the event loop in?

annevk · 2017-02-07T15:56:04Z

We're okay with circular dependencies between specs I guess.

Yeah, we crossed that bridge long ago.

Would defining that as a map event loop → list of transaction cleanup jobs in IDB be appropriate?

That sounds reasonable to me. Or we make the event loop available as "current event loop" or some such. Not sure what's better, but event loop is kinda a global variable.

annevk · 2017-02-13T13:43:14Z

@domenic are you on board with the above plan? @smaug----?

domenic · 2017-02-13T15:26:44Z

Yes

annevk · 2017-02-13T17:07:08Z

Updated. Still need a URL from Indexed DB for the term. @inexorabletash I haven't passed event loop explicitly. You can get it from the current settings object easily enough.

smaug---- · 2017-02-13T17:21:06Z

Sounds reasonable.

inexorabletash · 2017-02-13T17:38:14Z

Yeah, I'll try and get to a PR for IndexedDB this week for my end of the hook.

domenic

LGTM, will tag "do not merge yet" so we don't forget to update the href.

inexorabletash · 2017-02-13T19:08:52Z

w3c/IndexedDB#145 is my first stab at the Indexed DB side of things.

@smaug---- - can you take a look at the tests in web-platform-tests/wpt#4643 ?

inexorabletash · 2017-03-07T20:37:27Z

Landed tests in web-platform-tests/wpt@57aa2ac

inexorabletash · 2017-03-07T20:42:24Z

I've landed the Indexed DB change - the hook is now live:

https://w3c.github.io/IndexedDB/#cleanup-indexed-database-transactions

Add a post-microtask-job list to event loops

a2ab09a

Fixes #2279.

domenic requested changes Feb 2, 2017

View reviewed changes

smaug---- approved these changes Feb 3, 2017

View reviewed changes

Rework this to depend on IDB instead (note TODO)

53f857e

domenic approved these changes Feb 13, 2017

View reviewed changes

domenic added the do not merge yet Pull request must not be merged per rationale in comment label Feb 13, 2017

inexorabletash mentioned this pull request Feb 13, 2017

Implement transaction cleanup hook for [HTML] w3c/IndexedDB#145

Merged

remove TODO (note that IDB needs to merge first still)

453388b

inexorabletash mentioned this pull request Mar 4, 2017

IndexedDB: Add tests for HTML monkey patches web-platform-tests/wpt#4643

Merged

annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Mar 8, 2017

annevk merged commit 915dae1 into master Mar 8, 2017

annevk deleted the annevk/post-microtask branch March 8, 2017 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a post-microtask-job list to event loops #2310

Add a post-microtask-job list to event loops #2310

annevk commented Feb 1, 2017

inexorabletash commented Feb 2, 2017

annevk commented Feb 2, 2017

domenic left a comment

domenic Feb 2, 2017

annevk Feb 2, 2017

domenic Feb 2, 2017

domenic Feb 2, 2017

annevk Feb 2, 2017

domenic Feb 2, 2017

annevk Feb 2, 2017

domenic Feb 2, 2017

smaug---- left a comment

smaug---- Feb 3, 2017

inexorabletash Feb 3, 2017

smaug---- Feb 4, 2017

inexorabletash Feb 4, 2017

annevk commented Feb 7, 2017

inexorabletash commented Feb 7, 2017

annevk commented Feb 7, 2017

annevk commented Feb 13, 2017

domenic commented Feb 13, 2017

annevk commented Feb 13, 2017

smaug---- commented Feb 13, 2017

inexorabletash commented Feb 13, 2017

domenic left a comment

inexorabletash commented Feb 13, 2017

inexorabletash commented Mar 7, 2017

inexorabletash commented Mar 7, 2017

Add a post-microtask-job list to event loops #2310

Add a post-microtask-job list to event loops #2310

Conversation

annevk commented Feb 1, 2017

inexorabletash commented Feb 2, 2017

annevk commented Feb 2, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaug---- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Feb 7, 2017

inexorabletash commented Feb 7, 2017

annevk commented Feb 7, 2017

annevk commented Feb 13, 2017

domenic commented Feb 13, 2017

annevk commented Feb 13, 2017

smaug---- commented Feb 13, 2017

inexorabletash commented Feb 13, 2017

domenic left a comment

Choose a reason for hiding this comment

inexorabletash commented Feb 13, 2017

inexorabletash commented Mar 7, 2017

inexorabletash commented Mar 7, 2017