#15321: Only one tab writes to the DB #382

BeeMargarida · 2023-10-03T10:21:39Z

Details

This is the 1st part solution to fix the problem of the linked issue. The links to the broader discussion are below. This PR is big because it contains a RN app that contains an example for the e2e tests.

The goal of this PR is to change Onyx execution so that when there are several tabs open, only one of them can write to the DB. This is done by defining a leader tab, and all other tabs defer their writes to this one leader.

Changes included in this PR:

All writes to the DB are made by the leader tab - this involves using the BroadcastAPI to communicate each set, multiSet, merge and clear to the leader tab.
Ported ActiveClientManager to react-native-onyx and used BroadcastAPI instead of the indexedDB to reach a consensus between tabs about who is the leader (the last tab to be open). It uses timestamps in the messages to check that the last tab to send the message is the leader. If for a change the timestamps are the same, the client ID of the tab is used as a tiebreaker.
When the leader tab is closed, a new consensus is reached about the new leader tab. The old leader tab communicates that it is closing so that the others can try to reach a consensus.
The leader tab is listening to all write requests from the other tabs and execute them once they are received.
The leader tab does not execute writes to the DB while clear is running. These writes are ignored while the clear is running
The callback for the Onyx.clear is communicated. This is necessary to allow actions to execute after clear, so a non-leader needs a way of executing those actions without having access to the promise. So, a new Onyx.onClear should be set with the callback to execute after the Onyx.clear completes.

The GH comment with the explanation: Expensify/App#15321 (comment)

The doc with the discussion: https://docs.google.com/document/d/1Psr4Q4A2uH2b7l9CKBuB_dBT-15V4lJ7SOaH4uQOve4/edit

Related Issues

GH_LINK: Expensify/App#15321

Automated Tests

Added e2e tests using playwright. To run them:

npm install (should install onyx dependencies and the e2e app as well)
npm run e2e or npm run e2e-ui to run the e2e tests

Current result:

Linked PRs

This will require future changes in newDot, since ActiveClientManager is now available in react-native-onyx.

github-actions · 2023-10-03T10:21:52Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

BeeMargarida · 2023-10-03T10:22:52Z

I have read the CLA Document and I hereby sign the CLA

lib/ActiveClientManager/index.web.js

lib/Onyx.js

lib/broadcast/index.native.js

lib/storage/WebStorage.js

tests/e2e/app/package.json

tests/e2e/simple.spec.ts

package.json

lib/Str.js

lib/ActiveClientManager/index.web.js

rezkiy37

Left comments. However, in general, great job!

lib/ActiveClientManager/index.web.js

lib/Onyx.js

lib/ActiveClientManager/index.d.ts

tests/e2e/app/src/App.js

tests/e2e/app/src/Main.js

roryabraham · 2023-10-30T18:20:59Z

@koko57 if you have questions or need clarification on any of my suggestions just let me know 🙂

koko57 · 2023-11-08T10:14:26Z

@roryabraham Sorry, I had to take care of my other issues, but I'll address the rest of the comments soon

roryabraham · 2023-11-15T01:22:31Z

@koko57 thanks for pushing this forward. Please ping me when it's ready for review

koko57 · 2023-11-15T14:50:55Z

@roryabraham I think it's ready for re-review. On separate channels I commented here #382 (comment) - do we really need it? For the unsubscribe logic - as we no longer need to subscribe so many times, I also think it's unnecessary: #382 (comment). Let me know what do you think about it 🙂

roryabraham

I still think we need to add some unsubscribe logic. Even if there are fewer subscriptions, we're never cleaning them up which means we're propagating data to tabs that no longer exist unnecessarily.

.github/workflows/e2e.yml

lib/ActiveClientManager/index.web.js

koko57 · 2023-11-27T17:58:05Z

I still think we need to add some unsubscribe logic. Even if there are fewer subscriptions, we're never cleaning them up which means we're propagating data to tabs that no longer exist unnecessarily.

@roryabraham I checked it once again and if I understand this logic correctly, after closing tabs we no longer propagate the data to them. subscriptions array is separate for each tab and contains two callbacks - the one that we push while subscribing to in Onyx.init() and the second one in ActiveClientManager.init(). Both callbacks are then fired when a message from the channel is received, message object is passed to them both but they are looking for and handling different data types. The one from Onyx.init() (subscribeToEvents()) for data types to write to Onyx, the one from ActivveClientManager.init() for new tab selection. And we want these both callbacks for each tab. The part of code I removed ActiveClientManager.subscribeToClientChange(() => { subscribeToEvents(); (comment) added the callback from subscribeToEvents() so if we had i.e 8 tabs open the first one opened had one callback listening for leader change and 8 for listening for writing to Onyx. The second one had one callback less and so on and the leader tab had only these two different callbacks I mentioned earlier. After removing this part every tab has only these two callbacks from being open to being closed, no matter if we open a new tab or not. Each tab has its own subscriptions array, they don't share this array or store the info about subscriptions for other tabs, they are only communicating via sending/receiving messages through the broadcast.
I only wonder if we should clear the whole array and/or disconnect from the channel in 'beforeunload' but when a BroadcastChannel instance is closed (when closing the tab) event listeners (onmessage) should be removed as part of the cleanup process.

Let me know what do you think about it and correct me if I'm wrong with the logic 🙂

roryabraham · 2023-11-26T14:45:12Z

lib/ActiveClientManager/index.web.js

@@ -0,0 +1,99 @@
+/**
+ * When you have many tabs in one browser, the data of Onyx is shared between all of them. Since we persist write requests in Onyx, we need to ensure that


Just realizing that this comment is a bit out-of-date. This code no longer has anything to do with processing write requests.

roryabraham · 2023-12-01T22:34:40Z

Let's follow up ASAP with a PR to upgrade Onyx in E/App

koko57 · 2023-12-04T11:12:21Z

@roryabraham While trying to bump Onyx version in Expensify App I got this error

I've opened a draft PR for this - let me know if we can fix it this way

koko57 · 2023-12-08T22:25:33Z

@roryabraham wdyt?

roryabraham · 2023-12-09T03:10:18Z

Approved #434

chrispader · 2024-01-20T12:30:15Z

This PR caused a regression after the Onyx bump.

Fixing PR created here: #455

…eader_write" This reverts commit d858361, reversing changes made to d76bed7.

BeeMargarida added 16 commits September 4, 2023 14:52

feat: ActiveClientManager port

5c50c31

feat: broadcast

870d715

feat: only leader can write to DB

55a1394

Merge branch 'main' into feat/tab_leader_write

bb8be0a

fix: typo in import

c61749e

feat: e2e test app setup

fb4cd2f

test: playwright e2e tests setup

24e4cb8

revert: added unsubscrive

c78e71a

test: tabs e2e tests

d1fbe0d

feat: playwright config

e4a318e

revert: test ignore

d839fab

Merge branch 'main' into feat/tab_leader_write

8e7aa3b

chore: update package-lock

685c8f7

test: cleanup simple tests

ce15ce8

test: use getByRole instead of getByTestId

86fb581

test: comment out webkit from playwright config

37580f3

BeeMargarida added 2 commits October 3, 2023 11:27

fix: ignore e2e in jest config

88ee143

Merge branch 'main' into feat/tab_leader_write

63dd112

fabioh8010 reviewed Oct 4, 2023

View reviewed changes

rezkiy37 reviewed Oct 4, 2023

View reviewed changes

lib/ActiveClientManager/index.web.js Outdated Show resolved Hide resolved

BeeMargarida added 2 commits October 4, 2023 11:22

fix: set correct default values and better docs

c38b935

test: rename test file and suite

d12b355

rezkiy37 suggested changes Oct 4, 2023

View reviewed changes

BeeMargarida added 5 commits October 4, 2023 11:35

refactor: simplify conditional

76fd53f

fix: review fixes

8c4bfd1

fix: remove unsubscribe unused method

f0d3cf4

fix: small regression

b6db16e

docs: add comment

9efe824

koko57 added 7 commits November 9, 2023 11:00

Merge branch 'main' into feat/tab_leader_write

4f0638e

fix: resolve conflicts

cd9e9e9

feat: add e2e workflow

831b8bc

feat: add the info about tests to readme

7127185

fix: minor fix

f57f3f9

fix: remove unused directories

0dbd4b0

fix: resolve conflicts

dd5dea9

koko57 added 2 commits November 15, 2023 15:21

fix: remove unnecesary subscriber

cdb2680

fix: resolve conflicts

833d39a

fix: resolve conflicts

d8a1e22

koko57 mentioned this pull request Nov 21, 2023

[HOLD #34289] [$2000] The app crashes when the user is logged into multiple tabs and logs out of one of the tabs Expensify/App#15321

Closed

6 tasks

roryabraham requested changes Nov 21, 2023

View reviewed changes

.github/workflows/e2e.yml Outdated Show resolved Hide resolved

lib/ActiveClientManager/index.web.js Outdated Show resolved Hide resolved

lib/ActiveClientManager/index.web.js Outdated Show resolved Hide resolved

lib/ActiveClientManager/index.web.js Outdated Show resolved Hide resolved

fix: apply requested changes

c72152c

roryabraham approved these changes Dec 1, 2023

View reviewed changes

roryabraham merged commit d858361 into Expensify:main Dec 1, 2023
4 checks passed

koko57 mentioned this pull request Dec 4, 2023

fix: remove postinstall script #433

Closed

koko57 mentioned this pull request Dec 4, 2023

Feat/tab leader write fix #434

Merged

roryabraham mentioned this pull request Dec 24, 2023

Bump Onyx (Fix Replay effect) Expensify/App#33554

Merged

50 tasks

chrispader pushed a commit to margelo/react-native-onyx that referenced this pull request Jan 23, 2024

Revert "Merge pull request Expensify#382 from BeeMargarida/feat/tab_l…

fc18ba6

…eader_write" This reverts commit d858361, reversing changes made to d76bed7.

chrispader mentioned this pull request Jan 23, 2024

Revert tab leader PRs #458

Merged

42 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#15321: Only one tab writes to the DB #382

#15321: Only one tab writes to the DB #382

BeeMargarida commented Oct 3, 2023 •

edited

Loading

github-actions bot commented Oct 3, 2023 •

edited

Loading

BeeMargarida commented Oct 3, 2023

rezkiy37 left a comment

roryabraham commented Oct 30, 2023

koko57 commented Nov 8, 2023

roryabraham commented Nov 15, 2023

koko57 commented Nov 15, 2023

roryabraham left a comment

koko57 commented Nov 27, 2023 •

edited

Loading

roryabraham Nov 26, 2023

roryabraham commented Dec 1, 2023

koko57 commented Dec 4, 2023 •

edited

Loading

koko57 commented Dec 8, 2023

roryabraham commented Dec 9, 2023

chrispader commented Jan 20, 2024 •

edited

Loading

		@@ -0,0 +1,99 @@
		/**
		* When you have many tabs in one browser, the data of Onyx is shared between all of them. Since we persist write requests in Onyx, we need to ensure that

#15321: Only one tab writes to the DB #382

#15321: Only one tab writes to the DB #382

Conversation

BeeMargarida commented Oct 3, 2023 • edited Loading

Details

Related Issues

Automated Tests

Linked PRs

github-actions bot commented Oct 3, 2023 • edited Loading

BeeMargarida commented Oct 3, 2023

rezkiy37 left a comment

Choose a reason for hiding this comment

roryabraham commented Oct 30, 2023

koko57 commented Nov 8, 2023

roryabraham commented Nov 15, 2023

koko57 commented Nov 15, 2023

roryabraham left a comment

Choose a reason for hiding this comment

koko57 commented Nov 27, 2023 • edited Loading

roryabraham Nov 26, 2023

Choose a reason for hiding this comment

roryabraham commented Dec 1, 2023

koko57 commented Dec 4, 2023 • edited Loading

koko57 commented Dec 8, 2023

roryabraham commented Dec 9, 2023

chrispader commented Jan 20, 2024 • edited Loading

BeeMargarida commented Oct 3, 2023 •

edited

Loading

github-actions bot commented Oct 3, 2023 •

edited

Loading

koko57 commented Nov 27, 2023 •

edited

Loading

koko57 commented Dec 4, 2023 •

edited

Loading

chrispader commented Jan 20, 2024 •

edited

Loading