Adopt terms from Storage spec #75

inexorabletash · 2021-08-12T19:48:50Z

Nominally, replace "origin" with "storage key". But practically speaking:

Associate a lock manager with a storage bottle
Acquire a bottle from an environment, a manager from the bottle, and associate locks/requests with a manager

Some references to "storage bucket" are retained in prose where it seems to add value and is more correct than "origin".

Fixes #74

Preview | Diff

inexorabletash · 2021-08-12T21:26:46Z

@asutherland, @saschanaz, @mkruisselbrink - I'd appreciate your feedback here.

inexorabletash · 2021-08-12T21:35:08Z

The text "There is an equivalence between the following..." and associated Issue may be removable now.

mkruisselbrink

I haven't really thought about/looked at the details, but some high level question

index.bs

annevk

Reading @mkruisselbrink's comments again some of mine might be duplicative, but from a "fresh" perspective. Hope that's okay.

index.bs

annevk · 2021-09-02T09:02:10Z

index.bs

@@ -159,29 +161,25 @@ Resource names starting with U+002D HYPHEN-MINUS (-) are reserved; requesting th
 ## Lock Managers ## {#lock-managers}
 <!-- ====================================================================== -->

-A user agent has a <dfn>lock manager</dfn> for each [=/origin=], which encapsulates the state of all [=lock-concept|locks=] and [=lock requests=] for that origin.
+A [=/user agent=] has a <dfn>lock manager</dfn> for each [=/storage bottle=], which encapsulates the state of all [=lock-concept|locks=] and [=lock requests=] associated with a [=/storage bucket=].


Should this be defined in the Storage standard? It seems a little weird for other specifications to extend or change these fundamental concepts.

Oops, that text as written is wrong. Conceptually, there's a lock manager per bucket, not bottle. This is a result of a refactor where it used to say "bucket" here. But read on...

annevk · 2021-09-02T09:04:49Z

index.bs

-</aside>
+1. Let |bottle| be the result of [=/obtaining a local storage bottle map=] given |environment| and "`web-locks`".
+1. If |bottle| is failure, then return failure.
+1. Return |bottle|'s associated [=/lock manager=].


One thing I'm not quite clear on is that each storage bottle has a lock manager, but it seems we only care about it for the "web-locks" bottle here. Does that mean it should perhaps not live directly on the bottle or am I missing something?

The way the Storage standard is written and algorithms exposed, without changes to the standard "client" specs like this can get only a bottle for a feature. I think the intent is that for something like Indexed DB the bottle would literally be the key/value map where data is stored - for IDB, presumably a map of names to databases.

In the case of locks here, there's a top level "manager" object. With the bottle construct, we could:

Have the manager live inside the bottle map, as the value of a single key

Rewrite the manager to store all of its state as key/value pairs within the bottle map.

Monkey-patch the bottle map to be the manager with additional state properties.

"Associate" a manager with the specific bottle

The last is done here, basically bolting on a lock manager to a bottle, which is not very formal. (And again, it used to say "bucket" here) The text here is again the result of a refactor where this used to say "bucket", but since buckets aren't exported from Storage, that doesn't work.

I don't have a strong preference, but I do find the "store all state as key/value pairs within the bottle map" to be somewhat awkward/constraining as a spec author. I would prefer that specs can be written without modifications to Storage, i.e. we do keep the correct layering.

Have any other specs adopted the bottle mechanism? I put up an early PR for IDB but haven't looked at it in a while. With no changes to bottle, my preference would probably be to have keep the lock manager as-is, as a single value within the bottle. It seems like overkill, but spec objects are cheap. :)

HTML has for localStorage/sessionStorage. It might be more straightforward there.

But the idea is that nobody gets to access a bottle or bucket directly as they are in the conceptual "storage process". APIs just get a proxy to some space within that process.

(Concretely, as per https://storage.spec.whatwg.org/#model you'd invoke https://storage.spec.whatwg.org/#obtain-a-local-storage-bottle-map which would return a proxy map and not a bottle or some such.)

Hmm, that helps point out the challenge. Things like the lock manager and IDB database connection queues and transaction manager conceptually run algorithms in the "storage process" itself, so we need specs to have more than just proxied access to a k/v store.

Even if we keep this abstract for now, it seems wrong that above we associate it with a bucket, but here we pretend it's associated with a bottle.

saschanaz · 2021-11-09T20:41:07Z

Replace lock set and request queue associations with an origin with a lock manager instead

This better matches the current Gecko implementation, can we do it in a separate PR to make this one shorter?

inexorabletash · 2021-11-09T20:53:48Z

Replace lock set and request queue associations with an origin with a lock manager instead

This better matches the current Gecko implementation, can we do it in a separate PR to make this one shorter?

SGTM!

saschanaz · 2021-11-09T20:54:40Z

Cool, are you willing to do it or should I go ahead?

inexorabletash · 2021-11-10T01:37:25Z

Cool, are you willing to do it or should I go ahead?

I'm sure you'll get to it before I will!

Factor turning on "Assume Explicit For" out of #75. Purely editorial, adding "blah/" as needed to satisfy Bikeshed. Note that "No 'dfn' refs found for 'aborted flag' with for='['AbortSignal']'." is currently reported, will be fixed by #86

More factoring of logic out of #75 - this pulls the logic for obtaining a lock manager (or failing for opaque origins) out of call sites into a new set of steps, taking an environment as input. This sets us up for the later work to support partitioning. A few other tiny wording/syntax tweaks are included as well, but there are no intended normative behavior changes.

inexorabletash · 2021-11-12T20:27:39Z

FYI, c/o a bunch of refactorings (thanks @saschanaz for PRs and reviews!) this PR is now a much more minimal change. I haven't actually updated any of the relevant parts about bottles/buckets/etc based on discussion above, though.

ObSeuss:

saschanaz · 2021-11-12T20:30:35Z

I need to get more familiar with the storage spec to fully understand this one, so for now I think @annevk is still the best person to review this.

annevk · 2021-11-16T09:12:23Z

index.bs

@@ -27,7 +27,8 @@ spec: html; urlPrefix: https://html.spec.whatwg.org/multipage/
            text: agent cluster; url: integration-with-the-javascript-agent-cluster-formalism
 spec: storage; urlPrefix: https://storage.spec.whatwg.org/
    type: dfn
-        text: storage shelf; url: storage-shelf
+        text: storage bucket; url: storage-bucket
+        text: storage bottle; url: storage-bottle


This shouldn't be needed. If you need these exported we should change Storage.

Those are not exported indeed. Filed whatwg/storage#153, let's solve it separately.

annevk · 2021-11-16T09:14:02Z

index.bs

-</aside>
+1. Let |bottle| be the result of [=/obtaining a local storage bottle map=] given |environment| and "`web-locks`".
+1. If |bottle| is failure, then return failure.
+1. Return |bottle|'s associated [=/lock manager=].


Even if we keep this abstract for now, it seems wrong that above we associate it with a bucket, but here we pretend it's associated with a bottle.

2677224a857e74bda9be05e9ced66501cdc6d957 Squashed and rebased d9ad8282c3527566208ac089b9f46f9856693f8a remove unnecessary changes

saschanaz

I think it's time to revisit this...

saschanaz · 2022-11-22T12:48:24Z

index.bs

@@ -171,32 +172,19 @@ Resource names starting with U+002D HYPHEN-MINUS (-) are reserved; requesting th
 ## Lock Managers ## {#lock-managers}
 <!-- ====================================================================== -->

-A [=/user agent=] has a <dfn>lock manager</dfn> for each [=/origin=], which encapsulates the state of all [=lock-concept|locks=] and [=lock requests=] for that origin.
+A [=/user agent=] has a <dfn>lock manager</dfn> for each [=/storage bucket=], which encapsulates the state of all [=lock-concept|locks=] and [=lock requests=] associated with a [=/storage bucket=].


Each bucket has bottles for each storage endpoint, so should this be storage bottle instead as Anne pointed out below?

I think the reason I left this as "bucket" is that there is other (non-normative) text that tries to describe that locks are intended for coordination across agents that share a storage bucket (including in a future world with multiple buckets per origin/storage key), so (at the time) I wanted to keep the bucket term here as context. But it sounds like that may be confusing for someone reading this then looking at the algorithm, even though it's a 1:1 relationship.

How about...

"A lock manager encapsulates the state of locks and lock requests. A lock manager is associated with a storage bottle for the Web Locks API local storage endpoint in each storage bucket."

Further wordsmithing is appreciated!

That phrase sounds better to me! One nit is that, the 1:1 relationship of the bottle/bucket and the lock manager is less clear than the current phrase.

My try:

"A lock manager encapsulates the state of locks and lock requests. Each storage bucket includes one lock manager through an associated storage bottle for the Web Locks API.

Does this sound better to you? 👀🤔 This way I feel like the relationship between the bucket and bottle is less clear, but that specific relationship is technically not in the scope of this spec...

saschanaz · 2022-11-22T12:54:26Z

index.bs

-1. Let |origin| be |environment|'s [=/origin=].
-1. If |origin| is an [=/opaque origin=], then return failure.
-1. Return |origin|'s associated [=/lock manager=].
+1. Let |bottle| be the result of [=/obtaining a local storage bottle map=] given |environment| and "`web-locks`".


As already pointed out, "obtain a storage bottle map" doesn't give us a bottle directly. This should at least obtain "the bottle associated with the proxy map".

That sounds good as a temporary bridge until we come up with something better.

saschanaz · 2022-11-22T12:55:46Z

index.bs

-</aside>
-
-<aside class=issue>
-Migrate this definition to [[HTML]] or [[Storage]] so it can be referenced by other standards.


Perhaps another issue to incorporate LockManager to the bottle (or somewhere) properly?

I think this would be done with proposed rewording above?

Yes unless @annevk disagrees. We still have a yet another issue. for how to get the lock manager properly from the given environment, though.

inexorabletash · 2022-11-29T21:02:28Z

Updated - @saschanaz take a look?

saschanaz

LGTM, thanks! (Sorry for no review for a year)

saschanaz · 2022-11-29T21:28:43Z

index.bs

@@ -27,7 +27,8 @@ spec: html; urlPrefix: https://html.spec.whatwg.org/multipage/
            text: agent cluster; url: integration-with-the-javascript-agent-cluster-formalism
 spec: storage; urlPrefix: https://storage.spec.whatwg.org/
    type: dfn
-        text: storage shelf; url: storage-shelf
+        text: storage bucket; url: storage-bucket
+        text: storage bottle; url: storage-bottle


Those are not exported indeed. Filed whatwg/storage#153, let's solve it separately.

SHA: ffddb56 Reason: push, by inexorabletash Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

inexorabletash force-pushed the storagekey branch from 97d578e to 3658b1d Compare August 12, 2021 21:19

mkruisselbrink reviewed Aug 12, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

annevk reviewed Sep 2, 2021

View reviewed changes

saschanaz mentioned this pull request Nov 11, 2021

Move queue map and held lock set to lock manager #90

Merged

inexorabletash force-pushed the storagekey branch from 90a0ce8 to e5924b7 Compare November 12, 2021 18:59

inexorabletash mentioned this pull request Nov 12, 2021

Editorial: Turn on 'Assume Explicit For' #92

Merged

inexorabletash mentioned this pull request Nov 12, 2021

Factor "obtain a lock manager" out of algorithms #94

Merged

inexorabletash force-pushed the storagekey branch from e5924b7 to 89397b7 Compare November 12, 2021 20:23

annevk reviewed Nov 16, 2021

View reviewed changes

inexorabletash added 2 commits July 6, 2022 11:28

git squash commit for storagekey.

edfabd7

2677224a857e74bda9be05e9ced66501cdc6d957 Squashed and rebased d9ad8282c3527566208ac089b9f46f9856693f8a remove unnecessary changes

Fix some minor issues pointed out in the review (but not the major ones)

692908a

inexorabletash force-pushed the storagekey branch from 512988a to 692908a Compare July 6, 2022 18:28

inexorabletash mentioned this pull request Aug 8, 2022

TPAC 2022 status report #111

Closed

saschanaz reviewed Nov 22, 2022

View reviewed changes

more refinement

43ce2e3

saschanaz mentioned this pull request Nov 29, 2022

Find a better approach to incorporate with other specs; remove export hacks whatwg/storage#153

Open

saschanaz approved these changes Nov 29, 2022

View reviewed changes

inexorabletash merged commit ffddb56 into main Nov 29, 2022

github-actions bot added a commit that referenced this pull request Nov 29, 2022

Adopt terms from Storage spec (#75)

615ae49

SHA: ffddb56 Reason: push, by inexorabletash Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

saschanaz deleted the storagekey branch December 1, 2022 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt terms from Storage spec #75

Adopt terms from Storage spec #75

inexorabletash commented Aug 12, 2021 •

edited by pr-preview bot

Loading

inexorabletash commented Aug 12, 2021

inexorabletash commented Aug 12, 2021

mkruisselbrink left a comment

annevk left a comment

annevk Sep 2, 2021

inexorabletash Sep 2, 2021

annevk Sep 2, 2021

inexorabletash Sep 2, 2021

annevk Sep 3, 2021 •

edited

Loading

inexorabletash Sep 7, 2021

annevk Nov 16, 2021

saschanaz commented Nov 9, 2021 •

edited

Loading

inexorabletash commented Nov 9, 2021

saschanaz commented Nov 9, 2021

inexorabletash commented Nov 10, 2021

inexorabletash commented Nov 12, 2021

saschanaz commented Nov 12, 2021

annevk Nov 16, 2021

saschanaz Nov 29, 2022

annevk Nov 16, 2021

saschanaz left a comment

saschanaz Nov 22, 2022

inexorabletash Nov 29, 2022 •

edited

Loading

saschanaz Nov 29, 2022

saschanaz Nov 22, 2022

inexorabletash Nov 29, 2022

saschanaz Nov 22, 2022

inexorabletash Nov 29, 2022

saschanaz Nov 29, 2022

inexorabletash commented Nov 29, 2022

saschanaz left a comment

saschanaz Nov 29, 2022

Adopt terms from Storage spec #75

Adopt terms from Storage spec #75

Conversation

inexorabletash commented Aug 12, 2021 • edited by pr-preview bot Loading

inexorabletash commented Aug 12, 2021

inexorabletash commented Aug 12, 2021

mkruisselbrink left a comment

Choose a reason for hiding this comment

annevk 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 Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz commented Nov 9, 2021 • edited Loading

inexorabletash commented Nov 9, 2021

saschanaz commented Nov 9, 2021

inexorabletash commented Nov 10, 2021

inexorabletash commented Nov 12, 2021

saschanaz commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inexorabletash Nov 29, 2022 • edited Loading

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

inexorabletash commented Nov 29, 2022

saschanaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inexorabletash commented Aug 12, 2021 •

edited by pr-preview bot

Loading

annevk Sep 3, 2021 •

edited

Loading

saschanaz commented Nov 9, 2021 •

edited

Loading

inexorabletash Nov 29, 2022 •

edited

Loading