Page MenuHomePhabricator

[lib] introduce context to process Outbound P2P Messages
ClosedPublic

Authored by kamil on Mon, May 20, 4:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 14, 2:03 AM
Unknown Object (File)
Wed, Jun 12, 12:33 PM
Unknown Object (File)
Tue, Jun 11, 5:25 AM
Unknown Object (File)
Mon, Jun 10, 12:13 PM
Unknown Object (File)
Mon, Jun 10, 7:34 AM
Unknown Object (File)
Sun, Jun 9, 7:19 PM
Unknown Object (File)
Sun, Jun 9, 4:11 PM
Unknown Object (File)
Sun, Jun 9, 3:32 PM
Subscribers

Details

Summary

Please read inline comments, everything is explained in those

Depends on D12131

Test Plan

Testes in next diff on KeyserverStore example

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Mon, May 27, 4:52 AM
kamil edited the summary of this revision. (Show Details)
kamil added inline comments.
lib/handlers/db-ops-handler.react.js
43–45 ↗(On Diff #40656)

we have new OutboundP2PMessages - we have to start processing them

lib/tunnelbroker/peer-to-peer-context.js
46 ↗(On Diff #40656)

debug leftover

52 ↗(On Diff #40656)

We want to fetch all messages each time because:

  1. We need to make sure the order is correct, so when encrypting or sending we want to start from the oldest.
  2. If something went wrong in the previous iteration, we want to attempt to start again.

The only improvement here is that in getAllOutboundP2PMessages we should query all messages but not with sent status - creating a follow-up task for it.

54–61 ↗(On Diff #40656)

grouping all messages by device - when there is no session or we can not create a session with device we might want to skip the rest of messages

56–60 ↗(On Diff #40656)

we need to consider how to detect when messages for a device which is not in a peer list anymore can be deleted - creating a followup task for it

63 ↗(On Diff #40656)

this is used three times below - creating lambda to avoid code duplication

105 ↗(On Diff #40656)

We should filter if this is an error caused because of not existing session or a malformed session, but there is incompatibility in error messages thrown from the web and native, going to create a follow-up task for it.

This code is for a not existing session, when the session is malformed we should also add a code to resend messages, this will be addressed in Resending messages to peers.

126–127 ↗(On Diff #40656)

if this failed we could ignore that device, but on next run we'll attempt again

147 ↗(On Diff #40656)

we also might want to consider:

  1. Running this on app start
  2. Running this after a specific time (if there are no changes in DB but still want to send not yet send messages)
147–169 ↗(On Diff #40656)
  1. When there is no promise - spawn the promise processing messages
  2. When a promise is running - flip the switch (restartPromise) saying that if the promise is finished it needs to start again because something in DB changed (to avoid spawning multiple promises in parallel)
152 ↗(On Diff #40656)

debug leftover

lib/tunnelbroker/tunnelbroker-context.js
476 ↗(On Diff #40656)

I see this pattern somewhere in the codebase where Context render another one - I think here it makes sense as PeerToPeerProvider is done using Tunnelbroker, but I am open to update this

lib/types/sqlite-types.js
5–9 ↗(On Diff #40656)

adressed - the message was prepared to be sent to other peers but it's not encrypted. It was inserted into DB in the same transaction as making changes to the own store. For example, when adding a keyserver it's important to in the same SQL transaction update our keyserver_store and prepare messages to all devices at the same time. (I think this could be renamed but not sure about a better name).
encrypted - it was encrypted, encryption is done in the same transaction as the persisting crypto module to message order is also tracked on the client side, this means the message can be sent.
sent - the message that was sent to another peer (Tunnelbroker has it), we don't do anything about it, just wait for the confirmation (confirmation handled in peerToPeerMessageHandler)

tomek added inline comments.
lib/tunnelbroker/peer-to-peer-context.js
75 ↗(On Diff #40656)

Why do we parse the ciphertext?

126–127 ↗(On Diff #40656)

Is there a chance that this will succeed? Does it make sense to backoff or something?

lib/types/sqlite-types.js
5–9 ↗(On Diff #40656)

adressed is missleading. How about persisted?

This revision is now accepted and ready to land.Tue, May 28, 4:10 AM
lib/types/sqlite-types.js
5–9 ↗(On Diff #40656)

Might be good to add these as code comments. It seems useful for understanding the code

  • address review
  • rebase before landing
lib/tunnelbroker/peer-to-peer-context.js
56–60 ↗(On Diff #40656)
75 ↗(On Diff #40656)

In our case ciphertext is the stringified EncryptedData. If you think this is misleading we can rename it but this will require adding migration to update SQLite schema

105 ↗(On Diff #40656)
147 ↗(On Diff #40656)
lib/types/sqlite-types.js
5–9 ↗(On Diff #40656)

Might be good to add these as code comments. It seems useful for understanding the code

done

adressed is missleading. How about persisted?

done