refactor(mimefactory): separate rendering of message payload and sendable message - #8345
refactor(mimefactory): separate rendering of message payload and sendable message#8345link2xt wants to merge 1 commit into
Conversation
link2xt
changed the title
45102bc to
1956c42
Compare
5f3c57c to
f9c33de
Compare
f9c33de to
e314fc0
Compare
3d4610c to
b1b0e3f
Compare
ce43887 to
6abd90c
Compare
6abd90c to
e350dd6
Compare
a18ab11 to
2e20218
Compare
a39da96 to
1c19dd8
Compare
Agreed, there is no need to support chat-like features in unencrypted messages anymore. Hidden headers are from a time when we wanted to enable nice DC-to-DC communication via unencrypted messages with avatars etc. |
|
ftr, some history: we had late-mime-generation until 2018 and switched to early-mime-generation for various reasons, mainly speed and disappearing-messages, thid old core-c issue shows more detailes deltachat/deltachat-core#427 while performance might still be a concern, esp when there are only short time windows to get things out, iiuc, in this PR most data to be sent out are still generated early, so eg it is fine if database records the message originates from are deleted, this is important eg. for disappearing messages that time, however, mime-generation included encryption, so doing that late uses the state of the database when message goes out, not the state when send button is pressed, which led to all kind of issues. if i get it corectly, this is not the case with this PR, the mime |
|
I made some measurements on my 5 year old middle-class phone (Fairphone 3), compiled in release mode. FTR, this PR here doesn't switch to late encryption yet (it's just refactoring that will enable us to switch to late encryption). And yes, the plan is to switch to late encryption, but keep early mime generation. Once we switch to late encryption, the function Click to see the diff I used for measuringdiff --git a/src/chat.rs b/src/chat.rs
index 0ad2fd49d..9ee6bfc65 100644
--- a/src/chat.rs
+++ b/src/chat.rs
@@ -6,7 +6,7 @@
use std::io::Cursor;
use std::marker::Sync;
use std::path::{Path, PathBuf};
-use std::time::Duration;
+use std::time::{Duration, Instant};
use anyhow::{Context as _, Result, anyhow, bail, ensure};
use chrono::TimeZone;
@@ -2780,10 +2780,17 @@ async fn render_mime_message_and_pre_message(
let mut mimefactory_post_msg = mimefactory.clone();
mimefactory_post_msg.set_as_post_message();
+ let start = Instant::now();
let (queued_msg, side_effects) = Box::pin(mimefactory_post_msg.pre_render(context))
.await
.context("Failed to render post-message")?;
+ info!(
+ context,
+ "post-message pre_render took {:?}",
+ start.elapsed()
+ );
+ let start = Instant::now();
let rendered_msg = mimefactory::render_queued_mail(
queued_msg,
&public_key,
@@ -2792,12 +2799,20 @@ async fn render_mime_message_and_pre_message(
timestamp,
side_effects,
)?;
+ info!(
+ context,
+ "post-message render_queued_mail took {:?}",
+ start.elapsed()
+ );
let mut mimefactory_pre_msg = mimefactory;
mimefactory_pre_msg.set_as_pre_message_for(&rendered_msg);
+ let start = Instant::now();
let (queued_pre_msg, pre_side_effects) = Box::pin(mimefactory_pre_msg.pre_render(context))
.await
.context("pre-message failed to render")?;
+ info!(context, "pre-message pre_render took {:?}", start.elapsed());
+ let start = Instant::now();
let rendered_pre_msg = mimefactory::render_queued_mail(
queued_pre_msg,
&public_key,
@@ -2806,6 +2821,11 @@ async fn render_mime_message_and_pre_message(
timestamp,
pre_side_effects,
)?;
+ info!(
+ context,
+ "pre-message render_queued_mail took {:?}",
+ start.elapsed()
+ );
if rendered_pre_msg.message.len() > PRE_MSG_SIZE_WARNING_THRESHOLD {
warn!(
@@ -2818,7 +2838,10 @@ async fn render_mime_message_and_pre_message(
Ok((Some(rendered_pre_msg), rendered_msg))
} else {
+ let start = Instant::now();
let (queued_msg, side_effects) = Box::pin(mimefactory.pre_render(context)).await?;
+ info!(context, "pre_render took {:?}", start.elapsed());
+ let start = Instant::now();
let rendered_msg = mimefactory::render_queued_mail(
queued_msg,
&public_key,
@@ -2827,6 +2850,7 @@ async fn render_mime_message_and_pre_message(
timestamp,
side_effects,
)?;
+ info!(context, "render_queued_mail took {:?}", start.elapsed());
Ok((None, rendered_msg))
} |
| let (queued_mail, side_effects) = Box::pin(self.pre_render(context)).await?; | ||
| let rendered_mail = render_queued_mail( | ||
| queued_mail, | ||
| &public_key, | ||
| &secret_key, | ||
| from_addr, | ||
| timestamp, | ||
| side_effects, | ||
| )?; |
There was a problem hiding this comment.
render_queued_mail() is a blocking function (possibly blocking for multiple seconds) that is called without spawn_blocking() or block_in_place() both here and in multiple other places. This needs to be fixed in order not to block the executor
There was a problem hiding this comment.
Indeed, spawn_blocking() was removed from pk_encrypt(). I think, to protect from blocking the executor in such cases, we should use block_in_place() at the lower level, i.e. in pk_encrypt() while still calling spawn_blocking() here and in other high-level functions. So, even if spawn_blocking() is forgotten, the worst thing is other futures in the same task getting blocked, not other tasks. Nested block_in_place() should be fine (no-op), see discussion in tokio-rs/tokio#2327
There was a problem hiding this comment.
I wrapped calls into block_in_place() and added a note to the documentation of render_queued_mail.
There was a problem hiding this comment.
It's better to express this using the type system than the documentation that could be missed by function users, i.e. add some artificial token parameter to render_queued_mail() and wrappers for block_in_place() and spawn_blocking() that generate a token that can be passed into the function. At least this way you can grep all places in the code where such tokens are generated and check that blocking is fine there.
There was a problem hiding this comment.
Moved block_in_place() into pk_encrypt().
I don't know how we actually want to handle this, it does not look great to make functions that have nothing to do with async depend on tokio and it is not clear how much CPU usage is considered "blocking". Maybe we should move all block_in_place to lower levels, but then we should also do it for create_keypair, instead of using spawn_blocking in generate_keypair.
|
|
||
| if is_encrypted { | ||
| // Copy not protected headers to outer headers. | ||
| let (parsed_headers, _index) = mailparse::parse_headers(&raw_message)?; |
There was a problem hiding this comment.
FTR, we're first building the message and then parsing it again, but this parse_headers call takes just 5µs-80µs in my measurements (5 year old middle-class phone, release mode), and code-wise it seems like the easiest solution, so, it's good as-is.
There was a problem hiding this comment.
...although the only header we actually need is Chat-Is-Post-Message. For all the others (Subject, To, Chat-Version) it's already clear that they should be added and with which value. So, the code here could be slightly simplified by putting pre_message_mode on QueuedMail, and then always mechanically adding these four outer headers. Then we wouldn't need to parse the message here and iterate over all headers.
But it's fine as-is, too
|
|
||
| if is_encrypted { | ||
| // Copy not protected headers to outer headers. | ||
| let (parsed_headers, _index) = mailparse::parse_headers(&raw_message)?; |
There was a problem hiding this comment.
...although the only header we actually need is Chat-Is-Post-Message. For all the others (Subject, To, Chat-Version) it's already clear that they should be added and with which value. So, the code here could be slightly simplified by putting pre_message_mode on QueuedMail, and then always mechanically adding these four outer headers. Then we wouldn't need to parse the message here and iterate over all headers.
But it's fine as-is, too
| } else { | ||
| SeipdVersion::V1 | ||
| }; | ||
| let display_name = if is_securejoin_message && !is_encrypted { |
There was a problem hiding this comment.
IIUC, the naming of will_be_encrypted is correct. If the message is symmetrically-encrypted, then encryption_pubkeys is Some(vec![]). As @link2xt said elsewhere, would be a nice refactoring to put this into an enum instead in order to prevent such confusion.
The "vc-pubkey" and "vc-request-pubkey" messages don't use the function here. Instead, they are created by render_symm_encrypted_securejoin_message. This is why the code works as correctly. Thanks for writing a test for it!
| @@ -2022,84 +2293,26 @@ struct HeadersByConfidentiality { | |||
| /// See [`HeadersByConfidentiality`] for more info. | |||
| fn group_headers_by_confidentiality( | |||
There was a problem hiding this comment.
Maybe put a comment here that this function isn't really needed anymore and can be removed once we remove hidden headers?
598ac45 to
72d547a
Compare
Could you check if removing compression support from Asymmetric encryption is constant-time and should not depend on the message size, and symmetric AES encryption is supposed to be cheap and not taking seconds for 10 MB message, so I suspect the slowest part is OpenPGP compression. If it is that slow, it's one more reason to get rid of it. For encrypted attachments we don't need to worry about compatibility, so can even send them as binary MIME without base64-encoding, this will also reduce memory usage for the receivers and may help iOS. Large attachments are likely don't compress well as videos and images are already compressed, webxdcs are zip archives, and zlib compression cannot compress them further, so even just dropping compression already might be an improvement. The only reason for OpenPGP compression currently is compensating base64-encoding of binary attachments. Attachments are base64-encoded, then compressed, then signed and encrypted, then the whole encrypted messages is ASCII-armored. Compression negates the first base64-encoding, but we don't have to do it in the first place, mailparse at least theoretically supports binary attachments. |
|
Removing compression only helped a little bit, reducing the time from 3.5s to 2.6s. Additionally Ordering Rust to optimize for speed rather than binary size gets it down to 1.6s (I did not check how this affects the APK size). |
de45e20 to
95aba6d
Compare
af647e7 to
5eb92c2
Compare
5eb92c2 to
eef6ea4
Compare
hpk42
left a comment
hpk42
left a comment
There was a problem hiding this comment.
To me this looks good modulo a couple of medium/minor nits. The unencrypted MDN is probably most relevant, and then maybe the missing block_in_place. I am also in favor of merging this PR soon, and run main on some dev devices.
| // Never add outer multipart/mixed wrapper to MDN | ||
| // as multipart/report Content-Type is used to recognize MDNs | ||
| // by Delta Chat receiver and Chatmail servers | ||
| // allowing them to be unencrypted and not contain Autocrypt header | ||
| // without resetting Autocrypt encryption or triggering Chatmail filter | ||
| // that normally only allows encrypted mails. | ||
|
|
||
| // Hidden headers are dropped. | ||
| message |
There was a problem hiding this comment.
Pretty sure we want to get rid of unencrypted MDNs completely. Not implemented yet but it's better to just bail out here and drop the complex comment.
| // MDNs are only sent encrypted. Unencrypted MDNs are being | |
| // phased out but not fully prevented yet, so just refuse to render them here. | |
| bail!("unencrypted MDNs are not sent"); | |
| } |
| /// Symmetrically encrypt the message. | ||
| /// This is used for broadcast channels and for version 2 of the Securejoin protocol. | ||
| /// `shared secret` is the secret that will be used for symmetric encryption. | ||
| pub fn symm_encrypt_message( |
There was a problem hiding this comment.
doesn't this need block_in_place like pk_encrypt?
| msg.chat_id | ||
| .set_selfavatar_timestamp(context, now) | ||
| .await | ||
| .context("Failed to set selfavatar timestamp")?; |
There was a problem hiding this comment.
why is this changed from just logging an error in the background to aborting create_send_msg_jobs?
There was a problem hiding this comment.
I assume that the reason is that if one SQL statement errors, then there is usually not much point in continuing to try using the database (except there is a syntax error in the SQL statement, but then every test touching this code would fail)
| } | ||
| unprotected_headers.push(("To", hidden_recipients().into())); | ||
| } else if header_name == "chat-broadcast-secret" { | ||
| if is_encrypted { |
There was a problem hiding this comment.
not sure about just dropping this guard. the secret should never go out in unencrypted messages. Maybe some ensure_and_debug_assert!(...no-Chat-Broadcast-Secret..) in the unencrypted path makes sense?
| let date = chrono::DateTime::<chrono::Utc>::from_timestamp(self.timestamp, 0) | ||
| .unwrap() | ||
| .to_rfc2822(); | ||
| headers.push(("Date", mail_builder::headers::raw::Raw::new(date).into())); |
There was a problem hiding this comment.
Date now precedes To/Subject? nobody should care, just unusual for old-school readers like me :)
| .iter() | ||
| .map(|(name, addr)| { | ||
| Address::new_address( | ||
| if name.is_empty() { | ||
| None | ||
| } else { | ||
| Some(name.to_string()) | ||
| }, | ||
| addr.clone(), | ||
| ) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
sidenote: i really dislike such waste of vertical lines, as it impedes readability. It's not new code but while i am at it, i think something like self.to.iter().map(to_address).collect() would be preferrable (with an approprirate to_address helper).
There was a problem hiding this comment.
In addition, there already is fn new_address_with_name, although can't be used as .map(new_address_with_name) takes (&str, String) rather than (&String, &String)
| msg.chat_id | ||
| .set_selfavatar_timestamp(context, now) | ||
| .await | ||
| .context("Failed to set selfavatar timestamp")?; |
There was a problem hiding this comment.
I assume that the reason is that if one SQL statement errors, then there is usually not much point in continuing to try using the database (except there is a syntax error in the SQL statement, but then every test touching this code would fail)
| .iter() | ||
| .map(|(name, addr)| { | ||
| Address::new_address( | ||
| if name.is_empty() { | ||
| None | ||
| } else { | ||
| Some(name.to_string()) | ||
| }, | ||
| addr.clone(), | ||
| ) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
In addition, there already is fn new_address_with_name, although can't be used as .map(new_address_with_name) takes (&str, String) rather than (&String, &String)
eef6ea4 to
cd0eb02
Compare
…able message This change separates rendering into two separate steps: 1. Rendering of the message payload without the From, Date and Autocrypt headers. 2. Adding the From, Date and Autocrypt headers and possibly encrypting the message. The goal is to have serializable result of the first step that can be persisted in the database and sent later with any email address. This way it will be possible to send queued messages over any relay. This will make it possible not to remove all messages from the queue when the sending relay is changed. Currently changing `configured_addr` deletes everything from `smtp` table. This change is however only a refactoring and does not implement any features.
cd0eb02 to
6515b30
Compare
6 participants
This change separates rendering into two separate steps:
The goal is to have serializable result of the first step
that can be persisted in the database and sent later with any email address.
This way it will be possible to send queued messages over any relay.
This will make it possible not to remove all messages from the queue
when the sending relay is changed.
Currently changing
configured_addrdeletes everything fromsmtptable.This change is however only a refactoring and does not implement any features.
This is a refactoring PR in preparation for automatic relay failover.
As a side effect it also makes possible to change the Date of the message for #8112 if we decide on this approach (unlikely).
Serializable mail is currently called
mimefactory::QueuedMail. Everything except the public keys is trivially serializable, public keys should likely be serialized as recipient fingerprints rather than as OpenPGP certificates.I also noticed that we likely can remove the concept of "hidden headers" which are headers that are sent on the mulipart/mixed level of unencrypted messages. They are used to send
Chat-Editheaders and avatars in unencrypted messages. Sending avatars in unencrypted messages is not useful because they are not displayed anyway. And we can decide to make it impossible to edit and delete unencrypted messages. I have not changed anything in this PR, however, hidden headers work as before.