Sync broadcast lists (#4953) #4975

iequidoo · 2023-11-10T04:23:31Z

See commit messages. I'd like also to implement:

DONE: Sync of chat names.
DONE: Fix of the bug described in synchronize broadcast list members #4953 (comment).
Sync of chats removal (this one is definitely in another PR).

UPD: Actually #4953 (comment) isn't a bug, but just a missed feature. Done.

hpk42 · 2023-11-10T15:58:23Z

See commit messages. I'd like also to implement:
* Sync of chats removal.

please don't package this into the same PR -- getting broadcast lists synced is worthwhile on its own and we might then pull them out of "experimental" soon.

* Sync of chat names.

Do you mean the name a user gives to a contact manually?

iequidoo · 2023-11-10T21:23:06Z

Do you mean the name a user gives to a contact manually?

I'm about the chat.name column that can be changed using set_chat_name(). Currently it's not synchronised, so broadcast lists have different names on devices. I think this is the must for pulling a feature out of "experimental". As well as fixing the bug mentioned, otherwise messages from one device are not seen on others, though the broadcast list itself is seen because of synchronisation.

link2xt · 2023-11-11T19:37:19Z

src/chat.rs

@@ -417,7 +417,7 @@ impl ChatId {
            // TODO: For a 1:1 chat this currently triggers `Contact::unblock()` on other devices.
            // Maybe we should unblock the contact locally too, this would also resolve discrepancy
            // with `block()` which also blocks the contact.
-            chat.add_sync_item(context, ChatAction::Unblock).await?;
+            chat.sync(context, SyncAction::Unblock).await.ok();


Before .ok() add .log_err() so the error is logged. In the tests we will get some unconfigured errors printed, but it will not fail.

I'm constantly forgetting this because in my previous C life i used to logging errors at the lowest level, where they are generated and more details are known, passing the log context (prefix etc.) via TLS. It was not possible to report structured errors upstack, only int error codes (and there were lots of error handling built on these error codes). The downside is that many low-level errors are logged which are actually minor and even expected in systems with lots of hardware (there were ideas how to solve this, but not worked out enough).

But let's clarify our current approach. Afaiu errors should be logged either at the upper level or where they are ignored (one might say, turn into warnings) as here. Right?

Either log the error and ignore it if it is not critical and retry on the next iteration of IMAP loop does not make sense or may result into infinite loop trying to process the same message over and over, or bubble it up with ?.

The problem here is that error is thrown away without any logging at all.

The problem here is that error is thrown away without any logging at all.

Yes. My question is only about logging. So, errors should be logged before ignoring them, i.e. before .ok(). I guess, before handling them too. Are there other well-known conditions for that?

All errors should be either handled with if let Err() = (e.g. logging them into warn!), log_err().ok() or bubbled up with ?. Maybe we could write it down to https://github.com/deltachat/deltachat-core-rust/blob/main/CONTRIBUTING.md

src/receive_imf.rs

src/chat.rs

link2xt · 2023-11-12T05:45:23Z

src/chat.rs

@@ -4074,26 +4168,71 @@ pub(crate) async fn update_msg_text_and_timestamp(
    Ok(())
 }

+/// Set chat contacts by their addresses creating the corresponding contacts if necessary.
+async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[String]) -> Result<()> {


Why not

Suggested change

async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[String]) -> Result<()> {

async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[str]) -> Result<()> {

References to owned strings are generally not easy to work with.

error[E0277]: the size for values of type str cannot be known at compilation time
And i can't use &[&str] because i pass &Vec<String> here.

src/chat.rs

link2xt · 2023-11-12T06:10:04Z

src/receive_imf.rs

-                if let Err(err) = context.execute_sync_items(sync_items).await {
-                    warn!(context, "receive_imf cannot execute sync items: {err:#}.");
-                }
+                context.execute_sync_items(sync_items).await;


Re commit message:
"An error while executing an item mustn't prevent next items from being executed. There was a comment
that only critical errors like db write failures must be reported upstack, but in fact it's hard to
achieve in the current design, there are no error codes or so, so it's bug-prone. E.g.
ChatAction::Block and Unblock already reported all errors upstack. So, let's make error handling
the same as everywhere and just ignore any errors in the item execution loop. In the worst case we
just do more unsuccessful db writes f.e."

There are no error codes, but you can just warn! and return Ok() instead of generating an Err when the error is not critical. Then only critical errors are created in the whole codebase and if you get an error, you can always assume it is critical.

It still makes sense to never process the same message twice and log the error if sync message processing function failed in the middle.

The problem is that errors that are critical at the lower level (not sure that "critical" is the correct wording, but they are at least bubbled up) aren't critical at this level, e.g. some logic errors or ones caused by an invalid input. So, if we call some function that can fail for different reasons, incl. database write failures, we should always ignore errors from it, but the comment on execute_sync_items() said that db failures shouldn't be ignored, afaiu, because executing next items has no sense (they also would fail). This problem can't be solved w/o typed errors anyway, so i suggest not to complicate logic for now and just ignore errors in the item execution loop. I already made a couple of bugs with bubbling errors up from sync_alter_chat(), so better let's make the error handling the same as elsewhere.

link2xt · 2023-11-12T06:13:16Z

src/sync.rs

            }
+            .ok();


I think it was fine that Result was returned and turned into a warn! by the caller. But in any case, there should be .log_err() before ok(), otherwise errors such as a typo in SQL statement will be silently ignored and difficult to debug.

Added .log_err(). But i see no more reasons to return Result from here

link2xt · 2023-11-12T06:19:05Z

Looks good except for the changes to error handling potentially throwing errors away silently.

Sync chat contacts across devices for broadcast lists and groups. This needs the corresponding chat to exist on other devices which is not the case for unpromoted groups, so it fails for them now but it's only a warning and will work once creation of unpromoted groups is synchronised too.

It's sufficient if the local state is updated successfully, no need to fail the whole operation. Anyway delivery of sync messages and applying them on other devices are beyond of our control. If an error occurs when generating a sync messages, probably a log message is sufficient, no need to even show it to a user. As for the tests, anyway there are ones on synchronisation which perform necessary checks. Particularly, some sync messages can't be generated if an account is unconfigured. Adding the corresponding checks to the device synchronisation code (and maybe even more checks in the future) would complicate the code unnecessarily. Even errors caused by bugs in this code aren't a reason to fail a local operation.

An error while executing an item mustn't prevent next items from being executed. There was a comment that only critical errors like db write failures must be reported upstack, but in fact it's hard to achieve in the current design, there are no error codes or so, so it's bug-prone. E.g. `ChatAction::Block` and `Unblock` already reported all errors upstack. So, let's make error handling the same as everywhere and just ignore any errors in the item execution loop. In the worst case we just do more unsuccessful db writes f.e.

- Reduce cross-module dependencies. - Stop bloating the `sync` module while implementing synchronisation of more entities. - Now there's the only `ChatId` :)

link2xt force-pushed the main branch from ca190eb to d7aecab Compare November 10, 2023 17:56

iequidoo force-pushed the iequidoo/sync-chat branch 2 times, most recently from 040766a to 48f900e Compare November 10, 2023 23:04

iequidoo marked this pull request as ready for review November 10, 2023 23:29

iequidoo requested review from link2xt and r10s November 10, 2023 23:30

iequidoo force-pushed the iequidoo/sync-chat branch from 9731d0a to 2938cf2 Compare November 11, 2023 03:32

link2xt reviewed Nov 11, 2023

View reviewed changes

iequidoo mentioned this pull request Nov 12, 2023

synchronize broadcast list members #4953

Closed

iequidoo force-pushed the iequidoo/sync-chat branch from 26370e5 to bccbaa9 Compare November 12, 2023 04:02

link2xt reviewed Nov 12, 2023

View reviewed changes

iequidoo force-pushed the iequidoo/sync-chat branch from bccbaa9 to f8c7467 Compare November 12, 2023 06:43

link2xt approved these changes Nov 12, 2023

View reviewed changes

iequidoo force-pushed the iequidoo/sync-chat branch from f8c7467 to 9abe6c0 Compare November 13, 2023 07:37

iequidoo added 9 commits November 13, 2023 04:38

refactor: Rename Chat::add_sync_item() to sync()

3d4018c

feat: Sync creating broadcast lists across devices (#4953)

c1d45aa

test: Broadcast lists synchronisation (#4953)

ce0f9b1

refactor: Move chat-related code from sync to chat module

eef79c9

- Reduce cross-module dependencies. - Stop bloating the `sync` module while implementing synchronisation of more entities. - Now there's the only `ChatId` :)

feat: Sync Chat::name across devices (#4953)

956bb44

feat: Multi-device broadcast lists (#4953)

15aec23

iequidoo force-pushed the iequidoo/sync-chat branch from 9abe6c0 to 15aec23 Compare November 13, 2023 07:38

iequidoo merged commit ad5a5ad into main Nov 13, 2023
38 checks passed

iequidoo deleted the iequidoo/sync-chat branch November 13, 2023 08:00

hpk42 mentioned this pull request Nov 15, 2023

superflous sync messages for group "rename" #5011

Closed

hpk42 added the multi-device label Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync broadcast lists (#4953) #4975

Sync broadcast lists (#4953) #4975

iequidoo commented Nov 10, 2023 •

edited

Loading

hpk42 commented Nov 10, 2023

iequidoo commented Nov 10, 2023 •

edited

Loading

link2xt Nov 11, 2023

iequidoo Nov 12, 2023

link2xt Nov 12, 2023

iequidoo Nov 12, 2023

link2xt Nov 12, 2023

link2xt Nov 12, 2023

iequidoo Nov 13, 2023 •

edited

Loading

link2xt Nov 12, 2023

iequidoo Nov 13, 2023 •

edited

Loading

link2xt Nov 12, 2023

iequidoo Nov 13, 2023

link2xt commented Nov 12, 2023

	async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[String]) -> Result<()> {
	async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[str]) -> Result<()> {

Sync broadcast lists (#4953) #4975

Sync broadcast lists (#4953) #4975

Conversation

iequidoo commented Nov 10, 2023 • edited Loading

hpk42 commented Nov 10, 2023

iequidoo commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iequidoo Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iequidoo Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

link2xt commented Nov 12, 2023

iequidoo commented Nov 10, 2023 •

edited

Loading

iequidoo commented Nov 10, 2023 •

edited

Loading

iequidoo Nov 13, 2023 •

edited

Loading

iequidoo Nov 13, 2023 •

edited

Loading