Topic syncing and concurrency #335
Conversation
richardhuaaa
force-pushed
the
rich/topic-state
branch
from
1911291 to
f8eb60f
Compare
richardhuaaa
force-pushed
the
rich/topic-state
branch
from
ca4c850 to
336be08
Compare
| // TODO: We can handle errors in the transaction() function to make error handling | ||
| // cleaner. Retryable errors can possibly be part of their own enum | ||
| XmtpOpenMlsProvider::transaction(&mut self.store.conn()?, |provider| { | ||
| let is_updated = { |
There was a problem hiding this comment.
Wrapping this in a block, because the borrow needs to be ended before it's borrowed again somewhere else (e.g. when OpenMLS writes to the key store), or else there will be a runtime error. Don't love this solution but it works
There was a problem hiding this comment.
After discussion, will implement a WrappedConnection struct in the next PR that can solve this problem
| @@ -387,38 +377,36 @@ where | |||
| } | |||
| } | |||
|
|
|||
| pub fn process_messages(&self, envelopes: Vec<Envelope>) -> Result<(), GroupError> { | |||
| pub(crate) fn process_messages(&self, envelopes: Vec<Envelope>) -> Result<(), GroupError> { | |||
| let mut conn = self.client.store.conn()?; | |||
| let provider = self.client.mls_provider(&mut conn); | |||
| let mut openmls_group = self.load_mls_group(&provider)?; | |||
There was a problem hiding this comment.
Could merge this with the two lines above now that we don't need access to the provider or conn below this point
There was a problem hiding this comment.
I'm not sure what you mean by merging - like put it all in the same line? I think that just makes it less readable :/
There was a problem hiding this comment.
It is less readable, but it does return the connection to the pool sooner.
Not a big deal one way or the other.
xmtp_mls/src/client.rs
Outdated
|
|
||
| let envelopes = self | ||
| .api_client | ||
| .read_topic(topic, last_synced_timestamp_ns as u64) |
There was a problem hiding this comment.
The network query API is "greater than or equals", so I think we actually want this to be (last_synced_timestamp_ns + 1) to avoid getting duplicate messages we will end up skipping
There was a problem hiding this comment.
Changing this, but wouldn't it make more sense to have the network query API just be "greater than"? That seems like a much cleaner system overall
There was a problem hiding this comment.
I definitely wish I had done it that way when I first wrote the query API last year. The rationale was compatibility with Waku, which worked that way. Since we are using the generic query interface, we'd have to make the change for everyone (maybe with a flag). But yes, that would be better.
pull_from_topic)last_synced_payload_time(viaprocess_for_topic)These methods are agnostic to which topic, so can be used for both welcomes and group message processing, as well as any other case in the future. Unsure if they belong on
Clientor somewhere else.Some missing pieces:
last_synced_payload_timeor do we not?)