Page MenuHomePhabricator

[lib] Generate P2P messages based on actions
ClosedPublic

Authored by tomek on Jul 16 2024, 7:24 AM.
Tags
None
Referenced Files
F3140452: D12776.id42836.diff
Sun, Nov 3, 4:32 AM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Subscribers

Details

Summary

Inspired by https://phab.comm.dev/D12133. Each action can result in some messages that are defined in specs.

Depends on D12775

Test Plan

Tested in combination with the next diff. Checked if sending a DM operation results in generating P2P messages by adding some code calling a method from D12786.

Diff Detail

Repository
rCOMM Comm
Branch
dms (branched from palys-swm/dms)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Jul 16 2024, 7:42 AM

Can you provide some more context about this? I don't quite understand why it makes sense to start from an action... my initial thinking was that business logic would construct a DMOperation directly, rather than constructing an action that gets converted into a DMOperation

Can you provide some more context about this? I don't quite understand why it makes sense to start from an action... my initial thinking was that business logic would construct a DMOperation directly, rather than constructing an action that gets converted into a DMOperation

What do you mean by business logic? Is it the logic that lives in components, or in reducers, or maybe somewhere else?

I can see at least three possible places where DMOperations can be generated

  1. In reducers
  2. In specs called from a reducer - this implementation
  3. In components

I don't have a strong opinion about which one is better for us. Centralizing this logic sounds beneficial - they can be defined in some kind of specs, but we can call the specs from many places.

We also have some constraints in our solution that limit how we can handle the DMOperation

  1. Each OutboundP2PMessage needs to be put into the DB.
  2. All the items that go to our DB, need to be queued using queueDBOps.
  3. Then DBOpsHandler takes from the queue and saves items into the DB.

This means that without significant changes each OutboundP2PMessage needs to go through Redux. So if we decide to generate the DMOperations in components, we will need to introduce a new action that passes through Redux. This action will also need to modify the store and perform the same state modification that will happen on recipients. That's why starting with an action makes some sense - we already have the reducer logic that modifies the store based on it, and the only thing that is missing is creating the operation from it - similar to how we create DB operations.

What do you mean by business logic? Is it the logic that lives in components, or in reducers, or maybe somewhere else?

I was thinking of the logic in components.

I can see at least three possible places where DMOperations can be generated

  1. In reducers
  2. In specs called from a reducer - this implementation
  3. In components

I don't have a strong opinion about which one is better for us. Centralizing this logic sounds beneficial - they can be defined in some kind of specs, but we can call the specs from many places.

We also have some constraints in our solution that limit how we can handle the DMOperation

  1. Each OutboundP2PMessage needs to be put into the DB.
  2. All the items that go to our DB, need to be queued using queueDBOps.
  3. Then DBOpsHandler takes from the queue and saves items into the DB.

This means that without significant changes each OutboundP2PMessage needs to go through Redux. So if we decide to generate the DMOperations in components, we will need to introduce a new action that passes through Redux. This action will also need to modify the store and perform the same state modification that will happen on recipients. That's why starting with an action makes some sense - we already have the reducer logic that modifies the store based on it, and the only thing that is missing is creating the operation from it - similar to how we create DB operations.

Hmm... to me, I think it would be much more simple to have the components generate a DMOp, and then to have a utility function that calls processDMOperation on it, and put the result in a Redux action. That way, we avoid having to support multiple different kinds of actions. What do you think?

Hmm... to me, I think it would be much more simple to have the components generate a DMOp, and then to have a utility function that calls processDMOperation on it, and put the result in a Redux action. That way, we avoid having to support multiple different kinds of actions. What do you think?

Yeah, agree, it's simpler. It's similar to an updated solution I was thinking about. But there are some things we have to figure out:

  1. As of D12780 processDMOperation returns rawMessageInfos and updateInfos. But we also need to generate OutboundP2PMessages (generateMessagesToPeers function from this diff) - we probably should do that as a part of processDMOperation.
  2. We need to know the sending status of messages that aren't automatically retriable. We can have a function that takes OutboundP2PMessages and returns a promise that can be passed to dispatchActionPromise.
  3. Manual retries need IDs of failed messages - that is something relevant only for local messages. But we will need to modify them in a way that allows storing the IDs.

Figured sharing these makes us sure we're on the same page.

Don't create messages from actions

Would probably be a good idea to discuss this in my detail in our 1:1 tomorrow. One tradeoff neither of us has mentioned so far about using a single action for all business logic that generates DMOperations is that it might be debugging a little bit more painful.

But we also need to generate OutboundP2PMessages (generateMessagesToPeers function from this diff) - we probably should do that as a part of processDMOperation.

Hmmm... but doesn't processDMOperation get called on the peer to process the DMOperation sent by somebody else? It seems like we should only call generateMessagesToPeers on the sender's side.

  1. We need to know the sending status of messages that aren't automatically retriable. We can have a function that takes OutboundP2PMessages and returns a promise that can be passed to dispatchActionPromise.

A little confused about this one. Can you explain when / where we need to know this "sending status"? Will the business logic in components need to call this function and then pass it to dispatchActionPromise?

  1. Manual retries need IDs of failed messages - that is something relevant only for local messages. But we will need to modify them in a way that allows storing the IDs.

I'm confused about this one too. What needs to be modified? What sort of IDs need to be stored? I thought this data was available in the MessageStore already, but I might be missing something.

Would probably be a good idea to discuss this in my detail in our 1:1 tomorrow. One tradeoff neither of us has mentioned so far about using a single action for all business logic that generates DMOperations is that it might be debugging a little bit more painful.

Yes, definitelly.

But we also need to generate OutboundP2PMessages (generateMessagesToPeers function from this diff) - we probably should do that as a part of processDMOperation.

Hmmm... but doesn't processDMOperation get called on the peer to process the DMOperation sent by somebody else? It seems like we should only call generateMessagesToPeers on the sender's side.

Yes, we should generate messages only on sender's side. I was considering introducing a flag or something, but ultimately created a different solution D12786 where I create a new method sendDMOperation in PeerToPeerProvider.

  1. We need to know the sending status of messages that aren't automatically retriable. We can have a function that takes OutboundP2PMessages and returns a promise that can be passed to dispatchActionPromise.

A little confused about this one. Can you explain when / where we need to know this "sending status"? Will the business logic in components need to call this function and then pass it to dispatchActionPromise?

I'm not sure. E.g. in the input state container, we are checking the sending status of text messages. If our logic really needs that, we can call sendDMOperation and pass the result to dispatchActionPromise.

  1. Manual retries need IDs of failed messages - that is something relevant only for local messages. But we will need to modify them in a way that allows storing the IDs.

I'm confused about this one too. What needs to be modified? What sort of IDs need to be stored? I thought this data was available in the MessageStore already, but I might be missing something.

The missing data is the IDs of messages that are sent through tunnelbroker (OutboundP2PMessage). We're storing the messages in SQLite, but in order to retry them, we need their IDs. (there are some alternative approaches, e.g. sending messages to all the peers even if a message failed only for some - easier, but wasteful, and could be incorrect).

kamil added inline comments.
lib/shared/dm-ops/dm-op-utils.js
5–6 ↗(On Diff #42393)

can be merged

20 ↗(On Diff #42393)

What do you think about using uuid.v4() instead of our custom one?

48–52 ↗(On Diff #42393)

we should filter to exclude our own deviceID and avoid sending message to the device on which this code is executed (will fail on olm level)

49 ↗(On Diff #42393)

will this fail is currentUserInfo is null?

This revision is now accepted and ready to land.Jul 18 2024, 7:04 AM

Thanks for all of the explanations above, those all make sense. @tomek and I discussed this in our 1:1 today and settled on keeping this approach, at least for now.

tomek marked 4 inline comments as done.

Filter out this device ID

lib/shared/dm-ops/dm-op-utils.js
20 ↗(On Diff #42393)

I think it's a lot better to use the proper uuid.

49 ↗(On Diff #42393)

If the currentUserInfo is null, we're returning earlier.

if (!currentUserInfo?.id) {
  return [];
}