Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Explicit close() ? #13

Open
inexorabletash opened this issue Feb 26, 2018 · 7 comments
Open

Explicit close() ? #13

inexorabletash opened this issue Feb 26, 2018 · 7 comments
Labels
api A proposal for changing the API surface tentatively resolved We think we know the right answer, and it's in the spec, but we're open to changes

Comments

@inexorabletash
Copy link
Member

The current proposal relies on garbage collection to reclaim the StorageArea before the connection will be closed. Once #10 is resolved, this should not be observable since actions on areas should never block on upgrades (since the version is fixed).

Therefore I don't think there's a strong reason to complicate the API with close(), but I wanted to raise the issue.

@domenic
Copy link
Collaborator

domenic commented Feb 26, 2018

Notably the current API would mandate one always-open connection (at least once the module is imported once) for the storage built-in StorageArea. Do you think that's OK, given current IDB architectures?

@domenic
Copy link
Collaborator

domenic commented Feb 26, 2018

since actions on areas should never block on upgrades (since the version is fixed).

Is this still true even given full use of the IDB API on the database/store (via storageArea.backingStore)?

@inexorabletash
Copy link
Member Author

inexorabletash commented Feb 26, 2018

Not entirely sure what backingStore will yield, but presumably either the connection or the name; either way: no longer true. Example:

  • a = new StorageArea('foo') - holds a connection to async-local-storage-foo open until GC
  • r = indexedDB.open(a.backingStore.name, 2); r.onupgradeneeded = ... - blocked until a GCs
  • b = new StorageArea('foo') - blocked until previous is unblocked, and should then fail (since it wants v1 but the database has upgraded to v2)

EDIT: Saw that backingStore is more fleshed out in the README. No change to above.

@inexorabletash
Copy link
Member Author

inexorabletash commented Feb 26, 2018

Do you think that's OK, given current IDB architectures?

Other than the blocking issue when mixing the APIs (noted above): yes, it's fine to keep a connection open for the lifetime of the page.

Aside: the blocking behavior will span agents within the origin, in case that's not obvious.

Aside # 2: I'd actually missed the storage built-in. Given that connections do mandate work (e.g. hitting disk), and that a page is likely to include the module well before needing it... maybe the actual database open should be done lazily. I'll open a separate issue.

@domenic
Copy link
Collaborator

domenic commented Feb 28, 2018

I'm leaning toward having clear() do some sort of database-delete/close; that doesn't address the use case of wanting to free resources while still keeping data around for later, but it at least helps with resource uses in cases where you're saying that you don't care about this data at all anymore.

domenic added a commit that referenced this issue Feb 28, 2018
Closes #17. Closes #8 by giving you a way to recover from the database being broken if you clear() it before doing anything else with it. Helps with #13 by giving an explicit close at least for the cases where you no longer want to store any data in that StorageArea.
@domenic domenic added the api A proposal for changing the API surface label Feb 28, 2018
@domenic
Copy link
Collaborator

domenic commented Feb 28, 2018

I'm tentatively resolved in favor of not including close(), but it doesn't cost much. So I'll leave this issue open, with a "tentatively resolved" label, to signal to people that it's open for discussion.

@domenic domenic added the tentatively resolved We think we know the right answer, and it's in the spec, but we're open to changes label Feb 28, 2018
@pwnall
Copy link

pwnall commented Oct 17, 2018

FWIW, not having a close will make benchmarking more difficult. This is not a core use case, but I think it still matters to the API's long-term well-being.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api A proposal for changing the API surface tentatively resolved We think we know the right answer, and it's in the spec, but we're open to changes
Projects
None yet
Development

No branches or pull requests

3 participants