Page MenuHomePhabricator
Differential D13802

[lib] Make sure PeerToPeerMessageHandler is run after persisted state is read
ClosedPublic
Actions

Authored by angelika on Mon, Oct 28, 2:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 28, 4:21 PM
Unknown Object (File)
Mon, Oct 28, 4:21 PM
Unknown Object (File)
Mon, Oct 28, 4:21 PM
Unknown Object (File)
Mon, Oct 28, 4:20 PM
Subscribers
ashoat

Details

Reviewers
tomek
kamil
ashoat
Commits
rCOMMb51dd5e5ad70: [lib] Make sure PeerToPeerMessageHandler is run after persisted state is read
Summary

https://linear.app/comm/issue/ENG-9803/make-sure-peertopeermessagehandler-run-after-persisted-state-is-read

Depends on D13814

Test Plan
  1. Use logs to verify if PeerToPeerMessageHandler waits for persisted state
  2. Comment out sqliteAPI.removeInboundP2PMessages and send some messages and restart the app
  3. Errors like invalid operation should appear
  4. After this patch there should be less of them

Diff Detail

Repository
rCOMM Comm
Branch
graszka22/ENG-9803
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Mon, Oct 28, 2:48 PM
Herald added a subscriber: ashoat. · View Herald TranscriptMon, Oct 28, 2:48 PM
angelika updated this revision to Diff 45417.Mon, Oct 28, 2:52 PM

Rebase

Harbormaster completed remote builds in B32393: Diff 45415.Mon, Oct 28, 3:06 PM
Harbormaster completed remote builds in B32395: Diff 45417.Mon, Oct 28, 3:18 PM
angelika requested review of this revision.Mon, Oct 28, 3:18 PM
ashoat added a comment.Tue, Oct 29, 4:46 AM

Question about the test plan:

After this patch there should be less of them

Did some of errors still appear? What's your theory for why that is?

angelika added a comment.Tue, Oct 29, 4:52 AM
In D13802#385659, @ashoat wrote:

Question about the test plan:

After this patch there should be less of them

Did some of errors still appear? What's your theory for why that is?

Yes, they still appeared probably because this one: https://phab.comm.dev/D13792 has not landed yet. When I was testing that one the errors also disappeared.

ashoat accepted this revision.Tue, Oct 29, 5:22 AM

Makes sense, thanks for explaining!

This revision is now accepted and ready to land.Tue, Oct 29, 5:22 AM
angelika updated this revision to Diff 45448.Tue, Oct 29, 6:58 PM

Rebase and implement requested changes

angelika edited the summary of this revision. (Show Details)Tue, Oct 29, 7:00 PM
angelika added a parent revision: D13814: [lib] Add `canProcess` parameter to `useActionsQueue`.
angelika added a child revision: D13792: [lib] Wait with processing inbound messages for Tunnelbroker connection.
Harbormaster completed remote builds in B32421: Diff 45448.Tue, Oct 29, 7:20 PM
angelika removed a parent revision: D13801: [lib] Move usePersistedStateLoaded() to lib package.Wed, Oct 30, 5:04 AM
angelika updated this revision to Diff 45459.Wed, Oct 30, 6:02 AM

Update commit message

ashoat added a comment.Wed, Oct 30, 6:04 AM

You can't update the commit message with arc diff. Instead, you should edit the revision on Phabricator, and then run arc amend locally to update the commit text based on what's on Phabricator. (I think this last step also gets run automatically as part of arc land.)

angelika added a comment.Wed, Oct 30, 6:08 AM
In D13802#386045, @ashoat wrote:

You can't update the commit message with arc diff. Instead, you should edit the revision on Phabricator, and then run arc amend locally to update the commit text based on what's on Phabricator. (I think this last step also gets run automatically as part of arc land.)

oh, ok, I wanted to do just arc amend, I thought I need to do this manually 🤦‍♀️

Harbormaster completed remote builds in B32432: Diff 45459.Wed, Oct 30, 6:20 AM
Closed by commit rCOMMb51dd5e5ad70: [lib] Make sure PeerToPeerMessageHandler is run after persisted state is read (authored by angelika). · Explain WhyWed, Oct 30, 6:30 AM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMMb51dd5e5ad70: [lib] Make sure PeerToPeerMessageHandler is run after persisted state is read.

Revision Contents
Changeset List

PathSize
lib/
tunnelbroker/
10 lines

Diff 45417

View Options

lib/tunnelbroker/peer-to-peer-message-handler.js

Loading...