Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async local storage #278

Closed
domenic opened this issue May 1, 2018 · 6 comments
Closed

Async local storage #278

domenic opened this issue May 1, 2018 · 6 comments

Comments

@domenic
Copy link
Member

domenic commented May 1, 2018

Hello TAG!

I'm requesting a TAG review of:

Further details (optional):

  • I have read and filled out the Self-Review Questionnare on Security and Privacy. The assessment is "No" to all, with the following additional notes:
    • "Does this specification introduce new state for an origin that persists across browsing sessions?" No new state is introduced. However, the existing mechanism of IndexedDB is layered on top of and reused.
    • "Does this specification persist data to a user’s local device?" Yes, using IndexedDB's mechanisms. It follows IndexedDB in its interaction with "clear browsing data" and similar.
    • Unlike IndexedDB, this feature specifically requires a secure context, as it is a new storage API.
  • I have reviewed the TAG's API Design Principles

You should also know that...

This is designed as a layered API; see #276 for that review request.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
@slightlyoff
Copy link
Member

slightlyoff commented Jul 26, 2018

Discussed with @ojanvafai at this week's F2F meeting. We didn't really cover the exact semantics of Async Local Storage -- the new mechanism for exposing features took up most of the discussion time.

@slightlyoff
Copy link
Member

Just dug into the transasction lifetimes of operations; if I read the spec correctly, everything serializes on previous operations completing. Is this likely to be a performance hazard?

@travisleithead
Copy link
Contributor

In general, we are quite happy with this API. In particular, this has great "layering"--not adding new magic, which is good. Also, this feature gets exposed through the new module import mechanism which is cool--first feature to be released into the platform that's not available by default in the global realm.

Some other feedback:

  • This API piggy-backs on IndexedDB. It uses a particular string name to register itself: we think it would be a good idea to reserve some kind of prefix/namespace to avoid author code stomping on potential future Layered APIs. It's not something that can be enforced by the API obviously, but we think coming up with something and making that an author note would be helpful and not hurtful. (There may also be optimizations possible in implementation if a known prefix is used, or so I'm told.)

  • Getting the list of keys, values, etc., could impact memory footprint in the case of many keys/values. For the future, it would be nice to extend the API to support async iterators (when they become available).

@cynthia
Copy link
Member

cynthia commented Oct 31, 2018

The known prefix "optimization" is in the event the trees get too big the namespace gives the implementations a bit of leeway to throw in tricks such as lazy loading by shattering the trees. @inexorabletash might have some extra comments on that feedback above.

@travisleithead
Copy link
Contributor

Paris F2F: Thanks for requesting a review! Closing as we don't have any further feedback. As always, ping us again if you'd like additional review as a result of changes. Thanks for flying TAG!

@domenic
Copy link
Member Author

domenic commented Jul 15, 2019

For posterity:

This API piggy-backs on IndexedDB. It uses a particular string name to register itself: we think it would be a good idea to reserve some kind of prefix/namespace to avoid author code stomping on potential future Layered APIs. It's not something that can be enforced by the API obviously, but we think coming up with something and making that an author note would be helpful and not hurtful. (There may also be optimizations possible in implementation if a known prefix is used, or so I'm told.)

I think this is done in the spec; the kv-storage: prefix is reserved.

When the spec graduates from incubation and is incorporated into the IndexedDB spec itself, we could add a note to the relevant IDB methods discouraging web developers from using the kv-storage: prefix directly. (Unless they are intentionally trying to build on top of KV-storage-created databases, as in this spec example.)

Getting the list of keys, values, etc., could impact memory footprint in the case of many keys/values. For the future, it would be nice to extend the API to support async iterators (when they become available).

This was addressed in the spec; we now use async iterators for keys/values/entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants