[kvdb-web] indexeddb implementation #202

ordian · 2019-08-15T13:52:00Z

This is an alternative implementation for #182 based on IndexedDB (cc @tomaka).
Writes data both into memory and IndexedDB, reads from the IndexedDB on open.

Closes #182.

tomaka · 2019-08-16T07:36:14Z

kvdb-web/src/lib.rs

+}
+
+// TODO: is it safe for IdbDatabase?
+struct MakeSendSync<T>(T);


I generally use SendWrapper from here: https://docs.rs/send_wrapper/0.2.0/send_wrapper/
As far as I know, it is unfortunately actually not safe to implement Sync. If threads become usable one day, that will bite us.

Yes, this is technically unsafe unless we only write in a single thread. I'm not sure what's the best way forward though, this Send + Sync requirement is enforced by the KeyValueDB trait.

kvdb-web/Cargo.toml

We are forcing version alpha.17 in Substrate because of a compilation error with alpha.18. Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

tomaka · 2019-09-05T12:08:47Z

kvdb-web/src/indexed_db.rs

+		.expect_throw("IndexDB should be supported in your browser");
+
+	// TODO: don't hardcode version
+	let open_request = indexed_db.open_with_u32(name, /* version */ 1)


There are only two ways to handle this "version" thing here IMO: either we use the version, by passing it as parameter and adding a method to retrieve it later, or we don't pass any version at all.
I would go for no version at all.

Suggested change

let open_request = indexed_db.open_with_u32(name, /* version */ 1)

let open_request = indexed_db.open_with_u32(name)

Currently, if we reopen the database with more columns that it was opened previously, it will panic on open, when trying to create a transaction to read from a new column. Because a new column is expected to be created on version upgrade (try_create_object_stores).

We can pass the number of columns as a version, sounds reasonable?

Isn't it possible to dynamically check the number of columns, and add the missing ones?

I've tried that, but creating a column only possible onupgradeneeded:

The createObjectStore() method of the IDBDatabase interface creates and returns a new object store or index.
this method can be called only within a versionchange transaction.

I see! Then bump the version if we detect that the number of columns is different?
It might seem stupid, but I'd prefer to use things as they are intended to be used. In other words, the "version" should actually be a version and not a way for us to store a number.

Then bump the version if we detect that the number of columns is different?

The version is bumped only when we open the db with a higher version. And we can query the version on after either the open requests succeeds or onupgradeneeded is called (we specified a higher version that it is).

So I went with "by passing it as parameter and adding a method to retrieve it later" approach.

So I went with "by passing it as parameter and adding a method to retrieve it later" approach.

What worries me is that nobody is probably ever going to use this mechanism.
RocksDB automatically updates the number of columns if it doesn't match the number on disk, and I'm not sure it's a good idea to force users to manage a database version number just for IndexedDB.

What is going to happen in practice is that people will constantly forget to bump the database version number. RocksDB will work just fine, so nobody will realize, but IndexedDB will break.

In parity-ethereum we have a separate version file stored for RocksDB, see https://github.com/paritytech/parity-ethereum/blob/master/parity/db/rocksdb/migration.rs and the it refuses to open the newer version.

We could open the latest version of the db, see the version and the number of columns, and if the we need more columns, close it and reopen with a different number of version. But that sounds like a hack.

FWIW I've implemented this hacky solution.

kvdb-web/src/indexed_db.rs

* master: [plain_hasher] Migrate to 2018 edition (#213) [ethbloom] Improve ethbloom (#215) [rlp] fix nested unbounded lists (#203) stabilize parity-bytes in no_std environment (#212) Speed up hex serialization, support Serde `with`, and fix warnings (#208) [ethbloom, ethereum-types,kvdb] migrate to 2018 edition (#205) Introduce `ContractAddress` newtype instead of scheme enum (#200)

* master: [triehash] Migrate to 2018 edition (#214) [rlp] Add no_std support (#206)

* master: Bump version (#219) Find PendingIterator in Transaction Pool (#218) Modified AsRef impl for U-type macros (#196)

tomaka

LGTM

The version hack is obviously not great, but it's the least bad solution IMO.

tomaka · 2019-09-23T09:04:00Z

What's the next step? Does that need another review or can we merge?

ordian · 2019-09-23T09:36:42Z

It needs another review.

dvdplm

Minor nits and a few more tests would be good, but otherwise lgtm.

kvdb-web/src/indexed_db.rs

kvdb-web/src/lib.rs

dvdplm · 2019-09-24T14:04:51Z

Running wasm-pack test --headless --firefox on a mac yields:

output div contained:
    Loading scripts...

driver status: signal: 9
driver stdout:
    1569333767372	mozrunner::runner	INFO	Running command: "/Applications/Firefox.app/Contents/MacOS/firefox-bin" "-marionette" "-headless" "-foreground" "-no-remote" "-profile" "/var/folders/vh/svxftrqj1d583q97kl6ghqlh0000gn/T/rust_mozprofile.mak2ynYfurmW"
    1569333768042	Marionette	INFO	Enabled via --marionette
    1569333769292	Marionette	INFO	Listening on port 25808
    *** UTM:SVC TimerManager:registerTimer called after profile-before-change notification. Ignoring timer registration for id: telemetry_modules_ping

driver stderr:
    *** You are running in headless mode.
    2019-09-24 16:02:48.961 plugin-container[96640:19245410] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0xb53b, name = 'com.apple.tsm.portname'
    See /usr/include/servers/bootstrap_defs.h for the error codes.
    2019-09-24 16:02:49.249 plugin-container[96640:19245410] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0xa7b, name = 'com.apple.coredrag'
    See /usr/include/servers/bootstrap_defs.h for the error codes.
    2019-09-24 16:02:49.261 plugin-container[96640:19245410] unable to obtain configuration from file:///Library/Preferences/com.apple.ViewBridge.plist due to Error Domain=NSCocoaErrorDomain Code=257 "The file “com.apple.ViewBridge.plist” couldn’t be opened because you don’t have permission to view it." UserInfo={NSFilePath=/Library/Preferences/com.apple.ViewBridge.plist, NSUnderlyingError=0x116321f10 {Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted"}}
    2019-09-24 16:02:49.263 plugin-container[96640:19245412] +[NSXPCSharedListener endpointForReply:withListenerName:]: an error occurred while attempting to obtain endpoint for listener 'ClientCallsAuxiliary': Connection invalid
    2019-09-24 16:02:49.888 plugin-container[96641:19245540] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0xaf3b, name = 'com.apple.tsm.portname'
    See /usr/include/servers/bootstrap_defs.h for the error codes.
    2019-09-24 16:02:50.039 plugin-container[96641:19245540] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0xaf2b, name = 'com.apple.coredrag'
    See /usr/include/servers/bootstrap_defs.h for the error codes.
    2019-09-24 16:02:50.054 plugin-container[96641:19245540] unable to obtain configuration from file:///Library/Preferences/com.apple.ViewBridge.plist due to Error Domain=NSCocoaErrorDomain Code=257 "The file “com.apple.ViewBridge.plist” couldn’t be opened because you don’t have permission to view it." UserInfo={NSFilePath=/Library/Preferences/com.apple.ViewBridge.plist, NSUnderlyingError=0x1131380a0 {Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted"}}
    2019-09-24 16:02:50.056 plugin-container[96641:19245542] +[NSXPCSharedListener endpointForReply:withListenerName:]: an error occurred while attempting to obtain endpoint for listener 'ClientCallsAuxiliary': Connection invalid

error: some tests failed
error: test failed, to rerun pass '--test indexed_db'
Error: Running Wasm tests with wasm-bindgen-test failed
Caused by: failed to execute `cargo test`: exited with exit code: 1

Using Chrome also fails with:

$ wasm-pack test --headless --chrome
[INFO]: 🎯  Checking for the Wasm target...
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
[INFO]: ⬇️  Installing wasm-bindgen...
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running /Users/dvd/dev/parity/parity-common/target/wasm32-unknown-unknown/debug/deps/kvdb_web-8a0ae45a97d7609e.wasm
no tests to run!
     Running /Users/dvd/dev/parity/parity-common/target/wasm32-unknown-unknown/debug/deps/indexed_db-008424fae5ecf423.wasm
Running headless tests in Chrome with `/Users/dvd/Library/Caches/.wasm-pack/chromedriver-b5d7e439698af816/chromedriver`
driver status: signal: 9
driver stdout:
    Starting ChromeDriver 2.46.628411 (3324f4c8be9ff2f70a05a30ebc72ffb013e1a71e) on port 25869
    Only local connections are allowed.
    Please protect ports used by ChromeDriver and related test frameworks to prevent access by malicious code.

error: failed to find element reference in response
error: test failed, to rerun pass '--test indexed_db'
Error: Running Wasm tests with wasm-bindgen-test failed
Caused by: failed to execute `cargo test`: exited with exit code: 1

* master: [kvdb-memorydb] Migrated code to 2018 edition, updated parking_lot (#222) Migrated code to 2018 edition, updated dependencies (#221)

ordian · 2019-09-24T15:57:26Z

Not sure what to do with macOS, seems like wasm-pack issue to me.

dvdplm · 2019-09-24T17:12:21Z

Not sure what to do with macOS, seems like wasm-pack issue to me.

Ok, sounds like we should perhaps add a README where we list what combinations are "known good" (linux + chrome + ff I guess?) and call it a day? I mean, it doesn't seem like it should block the PR. An issue on the webpack repo linked from the README maybe?

kvdb-web/src/lib.rs

[kvdb-web] indexeddb implementation

dbbc4be

tomaka reviewed Aug 18, 2019

View reviewed changes

ordian and others added 6 commits August 18, 2019 18:05

Downgrade futures-preview

d183b02

We are forcing version alpha.17 in Substrate because of a compilation error with alpha.18. Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

[.travis.yml] disable headless tests on macOS

69e2d80

[kvdb-web] fix compiler warning

0c2c4e4

[kvdb-web] add a comment about sync unsafety

0410666

[.travis.yml] fix headless tests

3b0c9c0

[kvdb-web] put indexed_db into Mutex

3012a4a

ordian added the A0-pleasereview label Aug 27, 2019

ordian requested a review from tomaka September 4, 2019 15:52

ordian mentioned this pull request Sep 5, 2019

[kvdb] add an implementation for a browser #194

Closed

tomaka suggested changes Sep 5, 2019

View reviewed changes

ordian and others added 13 commits September 5, 2019 16:16

[kvdb-web] try creating only new columns

4c704f2

[kvdb-web] remove invalid comment

0a2a9d9

[kvdb-web] expose version

57af079

[kvdb-web] convert some expect_throws to expects

273dc37

[kvdb-web] introduce the error module

e9a0d6e

[kvdb-web] better error handling

73d3f0b

Merge branch 'master' into ao-kvdb-indexeddb

1e89da3

* master: [triehash] Migrate to 2018 edition (#214) [rlp] Add no_std support (#206)

[kvdb-web] remove unused error

ea86187

[kvdb-web] add license header to tests

95ac5f0

[kvdb-web] fix license copy-paste

46cf756

[kvdb-web] implement automatic version bump hack

45e2d05

Merge branch 'master' into ao-kvdb-indexeddb

3410968

* master: Bump version (#219) Find PendingIterator in Transaction Pool (#218) Modified AsRef impl for U-type macros (#196)

tomaka approved these changes Sep 16, 2019

View reviewed changes

ordian requested a review from dvdplm September 23, 2019 09:36

dvdplm approved these changes Sep 24, 2019

View reviewed changes

kvdb-web/src/indexed_db.rs Outdated Show resolved Hide resolved

kvdb-web/src/lib.rs Outdated Show resolved Hide resolved

kvdb-web/src/lib.rs Show resolved Hide resolved

kvdb-web/src/lib.rs Outdated Show resolved Hide resolved

kvdb-web/src/lib.rs Show resolved Hide resolved

ordian added 5 commits September 24, 2019 17:38

[kvdb-web] add docs to Database

dcc4969

[kvdb-web] mention reading the whole db in memory

54dc0f8

[kvdb-web] document why we reopen the db

803f35f

Merge branch 'master' into ao-kvdb-indexeddb

04fdee7

* master: [kvdb-memorydb] Migrated code to 2018 edition, updated parking_lot (#222) Migrated code to 2018 edition, updated dependencies (#221)

[kvdb-web] add a warning on transaction failure

ed5604a

[kvdb-web] fix typo in docs

1d95b73

dvdplm reviewed Sep 24, 2019

View reviewed changes

kvdb-web/src/lib.rs Outdated Show resolved Hide resolved

grammar pass by @dvdplm

c70b31d

ordian mentioned this pull request Sep 24, 2019

wasm-pack test --headless fails to run on macOS rustwasm/wasm-pack#717

Open

ordian merged commit 31b6479 into master Sep 25, 2019

ordian deleted the ao-kvdb-indexeddb branch September 25, 2019 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kvdb-web] indexeddb implementation #202

[kvdb-web] indexeddb implementation #202

ordian commented Aug 15, 2019 •

edited

tomaka Aug 16, 2019

ordian Aug 18, 2019 •

edited

tomaka Sep 5, 2019

ordian Sep 5, 2019

tomaka Sep 5, 2019

ordian Sep 5, 2019

tomaka Sep 5, 2019

ordian Sep 5, 2019

tomaka Sep 6, 2019 •

edited

ordian Sep 6, 2019

ordian Sep 16, 2019

tomaka left a comment

tomaka commented Sep 23, 2019

ordian commented Sep 23, 2019

dvdplm left a comment

dvdplm commented Sep 24, 2019 •

edited

ordian commented Sep 24, 2019

dvdplm commented Sep 24, 2019

	let open_request = indexed_db.open_with_u32(name, /* version */ 1)
	let open_request = indexed_db.open_with_u32(name)

[kvdb-web] indexeddb implementation #202

[kvdb-web] indexeddb implementation #202

Conversation

ordian commented Aug 15, 2019 • edited

Choose a reason for hiding this comment

ordian Aug 18, 2019 • edited

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

tomaka Sep 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka left a comment

Choose a reason for hiding this comment

tomaka commented Sep 23, 2019

ordian commented Sep 23, 2019

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm commented Sep 24, 2019 • edited

ordian commented Sep 24, 2019

dvdplm commented Sep 24, 2019

ordian commented Aug 15, 2019 •

edited

ordian Aug 18, 2019 •

edited

tomaka Sep 6, 2019 •

edited

dvdplm commented Sep 24, 2019 •

edited