fix: Send pre-message after successful sending of post-message (#8063)#8222
fix: Send pre-message after successful sending of post-message (#8063)#8222iequidoo wants to merge 2 commits into
Conversation
04832ec to
fed3a6a
Compare
| } | ||
|
|
||
| async fn handle_post_message( | ||
| async fn update_from_post_msg( |
There was a problem hiding this comment.
Decided to rename it to underline that only an already existing message is updated, to avoid more bugs caused by not fully handling post-messages
There was a problem hiding this comment.
What bug was caused by this?
There was a problem hiding this comment.
See #8222 (comment), probably the wrong condition was added there because handle_post_message() looks like hadndling post-messages in all cases. Actually it only updates existing messages
|
|
||
| // Maybe set logging xdc and add gossip topics for webxdcs. | ||
| for (part, msg_id) in mime_parser.parts.iter().zip(&created_db_entries) { | ||
| if mime_parser.pre_message != PreMessageMode::Post |
There was a problem hiding this comment.
Compat problem: missing iroh topic id
| context: &Context, | ||
| session: &mut Session, | ||
| ) -> Result<()> { | ||
| const PRE_MSG_WAIT_TIME: i64 = 30; |
There was a problem hiding this comment.
Compat problem: extra traffic for older versions
There was a problem hiding this comment.
I'd say it's fine, but let's ask @r10s:
This PR sends the pre-message only after successfully sending the post-message. This is to prevent a problem multiple users complained about: The pre-message arrives, but the post-message is delayed or rejected due to its size. On the receiver's side, the message will just say in "Downloading..." indefinitely, which is irritating.
Now, current versions of Delta Chat will download all Post-messages that do not have a pre-message in the assumption that the pre-message got lost. This means that a large message sent from the future version of DC will be downloaded unconditionally by the current version of DC, ignoring the download_limit setting, potentially unexpectedly wasting mobile data.
Do you think that this is fine?
If we do not want to accept this, we need to deploy this change over two releases: One release stops downloading Post-Messages that do not have a Pre-Message (i.e. the receiving part), and the next release sends the pre-message only after successfully sending the post-message (sending part).
fed3a6a to
42d09d4
Compare
42d09d4 to
f6eacd1
Compare
f6eacd1 to
2947c49
Compare
Also, send the message text in the post-message too, we can't support the workaround for duplicated message text shown in ancient versions anymore, otherwise newer versions won't show the message text at all. On the receiving side, download post messages w/o known pre-message after 30s to protect from lost messages. This isn't easy to test in Rust though because the `smtp` sending code requires an SMTP connection, so it's only tested (by already existing tests) that messages are queued in the right order. Another problem is that the `smtp` sending logic doesn't try to send any messages following a failed-to-send message until the retry limit is reached, so if there's smth wrong with a post-message, other unrelated messages are delayed, but this problem has already existed.
Messages are deleted from the inbox loop, so it should be interrupted. The SMTP doesn't need to be interrupted explicitly however, chat::sync() already does this internally.
2947c49 to
29a6f8d
Compare
| const PRE_MSG_WAIT_TIME: i64 = 30; | ||
| let now = tools::time(); | ||
| let rfc724_mids = context | ||
| .sql | ||
| .query_map_vec("SELECT rfc724_mid FROM available_post_msgs", (), |row| { | ||
| let rfc724_mid: String = row.get(0)?; | ||
| Ok(rfc724_mid) | ||
| }) | ||
| .query_map_vec( | ||
| " | ||
| SELECT rfc724_mid FROM available_post_msgs | ||
| WHERE timestamp<=? OR timestamp>? | ||
| ORDER BY timestamp, rowid | ||
| ", | ||
| (now.saturating_sub(PRE_MSG_WAIT_TIME), now), | ||
| |row| { | ||
| let rfc724_mid: String = row.get(0)?; | ||
| Ok(rfc724_mid) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This logic is becoming complex enough that I'm wondering whether it's worth it to guard against lost pre-messages. I mean, the large message getting through but not the small message is a pretty rare case, it creates extra edge cases, and it creates extra possible problems like accidentally wasting the user's mobile data and ignoring the download limit.
Maybe we should just remove this function. Then we also don't need the timestamp on available_post_msgs.
There was a problem hiding this comment.
the large message getting through but not the small message is a pretty rare case
I'm not sure. If the server loses messages randomly, regardless of their size, it may happen
| download_later.len(), | ||
| ); | ||
| let trans_fn = |t: &mut rusqlite::Transaction| { | ||
| let mut stmt = t.prepare("INSERT OR IGNORE INTO available_post_msgs VALUES (?)")?; |
There was a problem hiding this comment.
This old SQL statement will likely fail if the user upgrades (adding the column) and then downgrades again? Because SQL expects two values for inserting into the table.
This would require us to increase DCBACKUP_VERSION, but at this point, it really seems better to just not add the timestamp column.
There was a problem hiding this comment.
We can add a new table instead and add available post-messages to the both tables for the case of possible downgrade.
Maybe we need to add a STYLE.md rule that columns should be listed explicitly in INSERT statements, then the code will be more understandable and there will be no such problem for new columns having default values.
| context: &Context, | ||
| session: &mut Session, | ||
| ) -> Result<()> { | ||
| const PRE_MSG_WAIT_TIME: i64 = 30; |
There was a problem hiding this comment.
I'd say it's fine, but let's ask @r10s:
This PR sends the pre-message only after successfully sending the post-message. This is to prevent a problem multiple users complained about: The pre-message arrives, but the post-message is delayed or rejected due to its size. On the receiver's side, the message will just say in "Downloading..." indefinitely, which is irritating.
Now, current versions of Delta Chat will download all Post-messages that do not have a pre-message in the assumption that the pre-message got lost. This means that a large message sent from the future version of DC will be downloaded unconditionally by the current version of DC, ignoring the download_limit setting, potentially unexpectedly wasting mobile data.
Do you think that this is fine?
If we do not want to accept this, we need to deploy this change over two releases: One release stops downloading Post-Messages that do not have a Pre-Message (i.e. the receiving part), and the next release sends the pre-message only after successfully sending the post-message (sending part).
| "Message {rfc724_mid} is already in some chat or deleted." | ||
| ); | ||
| if mime_parser.incoming { | ||
| return Ok(None); |
There was a problem hiding this comment.
I'm having problems understanding why the changes here are necessary, can you clarify?
There was a problem hiding this comment.
If a post-message appears on IMAP, it should be deleted from smtp because it was sent successfully. Now this logic is common for all outgoing messages. One test started failing before i made this change, but i've forgotten which one exactly.
| } | ||
|
|
||
| async fn handle_post_message( | ||
| async fn update_from_post_msg( |
There was a problem hiding this comment.
What bug was caused by this?
Also, send the message text in the post-message too, we can't support the workaround for duplicated
message text shown in ancient versions anymore, otherwise newer versions won't show the message text
at all.
On the receiving side, download post messages w/o known pre-message after 30s to protect from lost
messages.
This isn't easy to test in Rust though because the
smtpsending code requires an SMTP connection,so it's only tested (by already existing tests) that messages are queued in the right order.
Another problem is that the
smtpsending logic doesn't try to send any messages following afailed-to-send message until the retry limit is reached, so if there's smth wrong with a
post-message, other unrelated messages are delayed, but this problem has already existed.
Fix #8063