[core data] State locks for concurrency control #26389

adamziel · 2020-10-22T11:56:32Z

An implementation of serial processing idea discussed in #26325

Testing instructions:

Start by not applying this PR. Instead apply demo PR Demo of concurrency control issue #26387.
Confirm you are able to reproduce the issue described in Demo of concurrency control issue #26387.
Apply this PR on top of Demo of concurrency control issue #26387.
Confirm the issue described in Demo of concurrency control issue #26387 is now gone.

Details:

The problem is that operations mutating the store (resolvers, saveEntityRecord) interfere with each other by changing the state used by other "concurrent" operations. My solution is to enable these operations to lock parts of the state tree that are expected to remain constant for the duration of the operation. For example saveEntityRecord("postType", "book", 3) would acquire a lock on a book with id=3 and nothing else.

More generally, interfering store operations should run sequentially. Each operation acquires a lock scoped to its state dependencies. If a lock cannot be acquired, the operation is delayed until a lock can be acquired. Locks are either exclusive or shared. If all operations request a shared lock, everything is executed concurrently. If any operation requests an exclusive lock, is it run only after all other locks (shared or exclusive) acting on the same scope are released.

The most complex part of this PR is the concept of scope. Locks are stored in a tree. Each node in the tree looks like this:

{
	"locks": [],
	"children": {}
}

Locks are stored like this:

{
	"locks": [ { "exclusive": false, /* data */ } ],
	"children": {}
}

A more complete, but still simplified, tree looks like this:

{
	"locks": [],
	"children": {
		"book": {
			"locks": [],
			"children": {
				1: {
					"locks": [],
					"children": {}
				},
				2: {
					"locks": [],
					"children": {}
				}
			}
		}
	}
}

Let's imagine we want to fetch a list of books. One way to control concurrency would be to acquire a shared lock on book. The lock will be granted once there are no exclusive locks on:

book itself
All its parents (root)
All its descendants (book > 1, book > 2)

In the case above we're good to grab the lock, let's do it then:

{
	"locks": [],
	"children": {
		"book": {
			"locks": [ { "exclusive": false, /* data */ } ],
			"children": {
				1: {
					"locks": [],
					"children": {}
				},
				2: {
					"locks": [],
					"children": {}
				}
			}
		}
	}
}

Let's imagine that another fetch was triggered to get a filtered list of entities. By checking the criteria above, it's okay to grab a shared lock on books so now we have two:

/* ... */
		"book": {
			"locks": [ { "exclusive": false, /* data */ },  { "exclusive": false, /* data */ } ],
		}
/* ... */

While these are running, the user triggered an update of book id=1. Now an update operation requests an exclusive lock. It will be able to acquire one once there are no locks at all on:

book > 1 itself
All its parents (root, book)
All its descendants (an empty list in this case)

Since there are two shared locks on book, the operation is delayed until both of them are released. Once that happens, an exclusive lock is granted on a specific book:

/* ... */
		"book": {
			"locks": [],
			"children": {
				1: {
					"locks": [  { "exclusive": true,  /* data */ } ],
					"children": {}
				},
			},
		},
/* ... */

If any fetch operation is triggered now, it will attempt to acquire a shared lock on book. Since one of book descendants holds an exclusive lock, fetch must wait until that lock is released.

This structure makes it quite easy to control concurrent reads/writes with, as I believe, any granularity. For example, if we wanted to make sure two fetch operations may run only if their query is different, each could acquire two locks: a shared lock on book to prevent clashing with writes, and an exclusive lock on an unrelated branch such as fetchByQuery > [query].

Other notes

This API is meant to be generic so that it could be re-used by any other package that needs state synchronization primitives.

cc @noisysocks

github-actions · 2020-10-22T12:52:07Z

Size Change: +2.52 kB (0%)

Total Size: 1.21 MB

Filename	Size	Change
`build/annotations/index.js`	3.77 kB	-3 B (0%)
`build/api-fetch/index.js`	3.42 kB	-2 B (0%)
`build/block-editor/index.js`	133 kB	-10 B (0%)
`build/block-library/index.js`	146 kB	-4 B (0%)
`build/block-serialization-default-parser/index.js`	1.87 kB	-1 B
`build/blocks/index.js`	48 kB	-8 B (0%)
`build/components/index.js`	171 kB	-12 B (0%)
`build/compose/index.js`	9.87 kB	+5 B (0%)
`build/core-data/index.js`	14.7 kB	+2.22 kB (15%)	⚠️
`build/data-controls/index.js`	1.12 kB	+352 B (31%)	🚨
`build/data/index.js`	8.74 kB	+3 B (0%)
`build/edit-navigation/index.js`	11.1 kB	-5 B (0%)
`build/edit-post/index.js`	305 kB	-2 B (0%)
`build/edit-site/index.js`	22.5 kB	-4 B (0%)
`build/edit-widgets/index.js`	26.2 kB	+1 B
`build/editor/index.js`	42.6 kB	-3 B (0%)
`build/format-library/index.js`	6.62 kB	-1 B
`build/keyboard-shortcuts/index.js`	2.52 kB	-2 B (0%)
`build/keycodes/index.js`	1.94 kB	+1 B
`build/list-reusable-blocks/index.js`	3.1 kB	-3 B (0%)
`build/notices/index.js`	1.77 kB	+3 B (0%)
`build/nux/index.js`	3.4 kB	-2 B (0%)
`build/plugins/index.js`	2.56 kB	+2 B (0%)
`build/priority-queue/index.js`	789 B	-2 B (0%)
`build/reusable-blocks/index.js`	3.05 kB	-4 B (0%)
`build/server-side-render/index.js`	2.77 kB	+1 B
`build/shortcode/index.js`	1.69 kB	-2 B (0%)
`build/url/index.js`	4.06 kB	+1 B
`build/viewport/index.js`	1.83 kB	-1 B

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/autop/index.js`	2.83 kB	0 B
`build/blob/index.js`	665 B	0 B
`build/block-directory/index.js`	8.71 kB	0 B
`build/block-directory/style-rtl.css`	943 B	0 B
`build/block-directory/style.css`	942 B	0 B
`build/block-editor/style-rtl.css`	11.2 kB	0 B
`build/block-editor/style.css`	11.2 kB	0 B
`build/block-library/editor-rtl.css`	8.96 kB	0 B
`build/block-library/editor.css`	8.96 kB	0 B
`build/block-library/style-rtl.css`	8.05 kB	0 B
`build/block-library/style.css`	8.05 kB	0 B
`build/block-library/theme-rtl.css`	792 B	0 B
`build/block-library/theme.css`	793 B	0 B
`build/block-serialization-spec-parser/index.js`	3.06 kB	0 B
`build/components/style-rtl.css`	15.3 kB	0 B
`build/components/style.css`	15.3 kB	0 B
`build/date/index.js`	31.8 kB	0 B
`build/deprecated/index.js`	769 B	0 B
`build/dom-ready/index.js`	571 B	0 B
`build/dom/index.js`	4.45 kB	0 B
`build/edit-navigation/style-rtl.css`	881 B	0 B
`build/edit-navigation/style.css`	885 B	0 B
`build/edit-post/style-rtl.css`	6.41 kB	0 B
`build/edit-post/style.css`	6.39 kB	0 B
`build/edit-site/style-rtl.css`	3.91 kB	0 B
`build/edit-site/style.css`	3.91 kB	0 B
`build/edit-widgets/style-rtl.css`	3.13 kB	0 B
`build/edit-widgets/style.css`	3.13 kB	0 B
`build/editor/editor-styles-rtl.css`	480 B	0 B
`build/editor/editor-styles.css`	482 B	0 B
`build/editor/style-rtl.css`	3.85 kB	0 B
`build/editor/style.css`	3.85 kB	0 B
`build/element/index.js`	4.62 kB	0 B
`build/escape-html/index.js`	735 B	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.16 kB	0 B
`build/html-entities/index.js`	623 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	712 B	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.32 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/primitives/index.js`	1.43 kB	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	13.4 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.22 kB	0 B

_{compressed-size-action}

adamziel · 2020-10-22T15:17:40Z

I'm tinkering with store-based storage, but any feedback on the direction is warmly welcome!

packages/core-data/src/actions.js

noisysocks

This is really cool! Not as nightmarishly complex as I had imagined it would be 🙂

Three suggestions—all of which I'm sure you're already working on:

Please update it to use redux state instead of a global variable.
Please add unit tests.
I was only able to grok how the code worked after I had read this PR's (excellent) description, so I suggest moving the "plumbing" functions (acquireLock, releaseLock, etc.) into their own file with a giant /** */ at the top containing what you wrote there.

.wp-env.json

packages/data-controls/src/index.js

packages/core-data/src/actions.js

packages/data-controls/src/index.js

adamziel · 2020-10-27T17:22:53Z

I will solve the conflicts tomorrow. This PR lacks comments, but hopefully all tests will pass. CC @youknowriad @mcsf @draganescu @noisysocks - let's discuss some more!

packages/data-controls/README.md

adamziel · 2020-10-28T15:32:16Z

These e2e test failures are unsettling - interestingly all the tests pass in my local environment 🤔

Edit: These failures are from tests missing something like await page.waitForSelector( '.notice-success' ); - they navigate to the next page immediately after clicking save. Everything works as expected in --puppeter-interactive mode so apparently applying this pr makes things last enough millisecond more for waitForSelector to be beeded in the headless mode.
Edit 2: That was a mixture of a bug in this PR AND things taking a few milliseconds than before.

adamziel · 2020-10-29T19:32:50Z

Tests failures are:

["Failed to inject axe-core into frame (about:blank)"],["Failed to inject axe-core into frame (about:blank)"]
Execution context was destroyed, most likely because of a navigation.

The first one is unrelated (#26527), the second one I'm not sure - anyone familiar with that one?

adamziel · 2020-11-04T12:11:53Z

Please ignore test failures for the purposes of reviewing this PR. Rebasing every day is tedious so I'm just giving up on it until it's closer to being merged. Also:

E2E tests failures I described earlier, I'm not sure if they actually mean anything.
Unit tests fail in test/integration.js - nothing is actually broken, it's just about the setup. It will be trivial to fix after [Concurrency issues] More integration tests #26661 lands.

…ent (cousin? :D) held a conflicting lock

Restore expectation of 5 assertions

packages/core-data/src/locks/README.md

adamziel added [Package] Core data /packages/core-data [Package] Data /packages/data [Package] Data Controls /packages/data-controls labels Oct 22, 2020

adamziel self-assigned this Oct 22, 2020

adamziel requested review from draganescu, noisysocks and youknowriad October 22, 2020 11:56

adamziel mentioned this pull request Oct 22, 2020

Concurrency control in core-data #26325

Closed

adamziel changed the title ~~Extremely rough first draft of operation queues~~ [Try] State locks for concurrency control Oct 22, 2020

adamziel commented Oct 22, 2020

View reviewed changes

packages/core-data/src/actions.js Outdated Show resolved Hide resolved

noisysocks reviewed Oct 23, 2020

View reviewed changes

adamziel force-pushed the add/concurrency-control branch from 270e760 to 9501daf Compare October 27, 2020 17:08

adamziel marked this pull request as ready for review October 27, 2020 17:20

adamziel requested a review from nerrad as a code owner October 27, 2020 17:20

adamziel requested a review from mcsf October 28, 2020 09:56

adamziel force-pushed the add/concurrency-control branch 2 times, most recently from b7befbc to 9287aa3 Compare October 28, 2020 13:08

draganescu reviewed Oct 28, 2020

View reviewed changes

packages/data-controls/README.md Show resolved Hide resolved

adamziel force-pushed the add/concurrency-control branch from 9287aa3 to 73e23df Compare October 29, 2020 12:40

adamziel requested review from ajitbohra, ntwb and talldan as code owners October 29, 2020 15:15

adamziel mentioned this pull request Nov 3, 2020

[@wordpress/core-data] getEntityRecords and saveEntityRecord don't sync data correctly #22127

Closed

adamziel force-pushed the add/concurrency-control branch from a141486 to 7756b49 Compare November 3, 2020 15:58

adamziel added 15 commits November 9, 2020 16:01

squash

a6eedcd

Lint

69188dc

Rebase actions and resolvers

4b9a929

Remove the usage of syncSelect

824a7bc

Update package-lock.json

8013dbc

Update package-lock.json

74018d9

Update package-lock.json

c35eaea

Fix a problem with lock not granted if a children of sibling of a par…

f3f3a5a

…ent (cousin? :D) held a conflicting lock

Fix multi-entity-saving.test.js

9c36ab5

Update multi-entity-saving.test.js

42266f9

Restore expectation of 5 assertions

Update required UUID version

4771975

Update package-lock.json

dda83b1

Formalize store path as separate arguments in the public API

775a020

Fix resolvers tests

7278fc0

Fix integration tests

ea37361

adamziel force-pushed the add/concurrency-control branch from 7756b49 to ea37361 Compare November 9, 2020 15:34

adamziel added 2 commits November 9, 2020 16:36

Mark the new public API as __unstable

95c9891

lint

5758cf5

youknowriad approved these changes Nov 10, 2020

View reviewed changes

adamziel merged commit 8ec3eac into master Nov 10, 2020

adamziel deleted the add/concurrency-control branch November 10, 2020 13:06

github-actions bot added this to the Gutenberg 9.4 milestone Nov 10, 2020

This was referenced Nov 10, 2020

Mark AWAIT_PROMISE as unstable API #26852

Merged

Demo of concurrency control issue #26387

Closed

Migrating experimental navigation and widgets screen to bulk REST API #24907

Closed

TimothyBJacobs reviewed Nov 11, 2020

View reviewed changes

packages/core-data/src/locks/README.md Show resolved Hide resolved

TimothyBJacobs reviewed Nov 11, 2020

View reviewed changes

packages/core-data/src/locks/README.md Show resolved Hide resolved

TimothyBJacobs reviewed Nov 11, 2020

View reviewed changes

packages/core-data/src/locks/README.md Show resolved Hide resolved

adamziel mentioned this pull request Nov 12, 2020

More appropriate docs for the locks module #26919

Merged

adamziel mentioned this pull request Oct 25, 2021

Performance: Improve opening of the block inserter via manual token-based analysis of reusable blocks #35763

Merged

7 tasks

[core data] State locks for concurrency control #26389

[core data] State locks for concurrency control #26389

Uh oh!

Conversation

adamziel commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamziel commented Oct 22, 2020

Uh oh!

Uh oh!

noisysocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamziel commented Oct 27, 2020

Uh oh!

Uh oh!

adamziel commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamziel commented Oct 29, 2020

Uh oh!

adamziel commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adamziel commented Oct 22, 2020 •

edited

Loading

github-actions bot commented Oct 22, 2020 •

edited

Loading

adamziel commented Oct 28, 2020 •

edited

Loading

adamziel commented Nov 4, 2020 •

edited

Loading