deltachat / deltachat-core

Delta.Chat C-Library with e2e chat-over-email functionality & Python bindings
https://c.delta.chat
Other
304 stars 26 forks source link

avoid late generation of mime-structure #427

Closed r10s closed 5 years ago

r10s commented 5 years ago

currently, the mime-messages are generated from the database with the plain-text information just before sending (let us call this late-mime-generation):

the text and other information to sent is inserted to the database, we get a database-id and this database-id is added to a job-table then. as soon as the job is executed, the mime-structure is generated)

while this works usually well, this has some downsides:

the alternative is early-mime-generation:

however, when it comes to single-sided ephemeral-messages, early-mime-generation may be a MUST and not an option.

the idea of single-sided ephemeral-messages is that they do not stay on the device longer than a given amount of time. if this time is 0, however, we get the problem that the network may not be available just in that moment - and we cannot try resending later without breaking the promise of not saving the message.

this dilemma could be solved by encrypting the message to sent only to the recipients, and not to the own key, which, however, required the job-table being more independent from the message-table.

however, there are still open questions:

hpk42 commented 5 years ago

great writeup ... few comments ...

On Thu, Nov 08, 2018 at 14:58 -0800, björn petersen wrote:

the alternative is early-mime-generation:

  • advantages/disadvantages are inversion from above

i think in flaky networks early-mime generation allows to do the fastest push out to the SMTP server. In the late-mime generation time is spent constructing a mime message (which in the case of longer files may be substantial?)

also i guess the core can with early-mime generation more easily support bots or other "compose-mail-views" preparing the mime structure differently than currently.

[need to delete message on sending device ...] this dilemma could be solved by encrypting the message to sent only to the recipients, and not to the own key, which, however, required the job-table being more independent from the message-table.

however, there are still open questions:

  • in a multi-device-setup, this message won't reach the other devices. this is not necessarily a problem, however, it must be indicated to the user in a way

i guess the lack of multi-device support with ephemeral messages might fit user expecations. In the concrete use cases we both know multi-device is not an issue.

  • these messages cannot be used in the SELF chat then

probably also a non-issue.

so all in all i think it's a very good plan! :) holger

r10s commented 5 years ago

thanks for the comment, esp. the flaky-network is a good point.

wrt to multi-device-ephemeral-messages or self-sent-ephemeral-messages i share the point that this is probably not an issue. just wanted to raise the question :)

r10s commented 5 years ago

an related issue is the request to cancel sending messages.

currently this works already for subsequent attempts, and, if possible, we should not make things worse on this point. see https://github.com/deltachat/deltachat-android-ii/issues/188

hpk42 commented 5 years ago

On Fri, Dec 21, 2018 at 07:03 -0800, björn petersen wrote:

an releated issue is the request to cancel sending messages, currently this works already for subsequent attempts, and, if possible, we should not make things worse on this point. see https://github.com/deltachat/deltachat-android-ii/issues/188

somewhere in libetpan there will be a chunked sending of data via smtp -- there could be a callback which passes the message-id/handle of what is currently send so that deltachat can make the decision with every chunk if a message continues to be send.

it's somewhat unrelated to early-mime generation, i think.

r10s commented 5 years ago

it's somewhat unrelated to early-mime generation, i think.

it's related in the sense that by the current late-mime-generation subsequent sendings will fail automatically when the msg_id is no longer available in the database. we just cannot generate the mime in this case currently.

with early-mime-generation, this is a bit different as the msg_id is just no longer needed on subsequent sendings.

so when the messages are not being sent as ephemeral messages, we may want to check if msg_id is still in the database on real sending time. or, maybe better, delete the corresponding job.

hpk42 commented 5 years ago

On Fri, Dec 21, 2018 at 08:49 -0800, björn petersen wrote:

it's somewhat unrelated to early-mime generation, i think.

it's related in the sense that by the current late-mime-generation subsequent sendings will fail automatically when the msg_id is no longer available in the database. we just cannot generate the mime in this case currently.

k.

with early-mime-generation, this is a bit different as the msg_id is just no longer needed on subsequent sendings.

right.

so when the messages are not being sent as ephemeral messages, we may want to check if msg_id is still in the database on real sending time. or, maybe better, delete the corresponding job.

my guess is that this could all be solved by passing a callback to libetpan's smtp-sending that asks back for each chunk if it shall continue, maybe also providing the bytes transferred so far. This could also help to detect situations of failed partial upload tries of large files.

r10s commented 5 years ago

it may be handy to get rid of the increation flag for a proper implementation of this feature

VP- commented 5 years ago

it may be handy to get rid of the increation flag for a proper implementation of this feature

Forwarded messages still need a way to know when their file is ready to send. Moving the flag from the file (blob directory) to the original message (e.g. a new state) will require an additional reference from a forwarded message to the original, to check the file state. Where can we put that additional reference? The choices seem to be SMTP job vs message, and a new column vs. a new param.

r10s commented 5 years ago

Forwarded messages still need a way to know when their file is ready to send.

that's a point. while the ".increation" files look hacky at the first glace, in fact, they solve a bunch of issues in a very simple way :)

VP- commented 5 years ago

Does that mean we're keeping the .increation files?

r10s commented 5 years ago

not sure, i think this needs some thinking. as this is not really an urgent issue but becomes more complicated as initially expected, maybe it is the best to postpone this a bit and target more low hanging fruits for now :)

the .increation stuff is also mainly releated to video recoding that will be re-implemented maybe in the nexts months; maybe this is a good time to re-target this issue.

r10s commented 5 years ago

to summarize: iirc the idea was to split the current dc_send_msg() into a dc_prepare_msg() and a dc_send_msg().

if dc_prepare_msg() is not called explicitly, this is done by dc_send_msg().

currently, instead of dc_prepare_msg(), the ui may create a file with the same plus .increation extension which avoids the message from being sent out; the core just tries again after some seconds. this is a bit hacky, however, has the advantage that messages can eg. be forwarded while in preparation. this would be more complicated with the two functions as sending may cause some messages to be sent ... sounds painful.

maybe we could just say that messages being in preparation cannot be forwarded. again, not sure :)

VP- commented 5 years ago

The removal of .increation files is now ready in PR #585.

I decided to store the information which links forwarded messages in a new parameter of the original message (DC_PARAM_PREP_FORWARDS).

One possible problem is the choice of the value for the new state DC_STATE_OUT_PREPARING, since now the states of a message are not monotonically increasing anymore. Inequality comparisons on the state (e.g. using msg->state < DC_STATE_OUT_PENDING to see if the message is not in the database yet) could break.

VP- commented 5 years ago

I've added a commit which actually implements early MIME generation. Now I have a conflict with the new dc_set_gossiped_timestamp() function. Which timestamp should it use (and therefore when should it be called)? During MIME generation, or after a successful sending via SMTP?

r10s commented 5 years ago

hm, i think, this is not easy to answer.

on key-changes or changes of the memberlist, dc_reset_gossiped_timestamp() is called which signals the mime-factory that keys of group members should be re-gossiped. once done, dc_set_gossiped_timestamp() is called.

we should also keep in mind that the current exponential backoff algorithm may result in messages being sent send out-of-order (messages may also arrive out-of-order, for other reasons, of course).

  1. if we set the timestamp on SMTP-sending, the actually sent mime-structure may be created with an old set of gossiped keys, and may not include the changes that were the reasons for dc_reset_gossiped_timestamp() being called.

  2. setting the timestamp on mime-generation gives more guarantee that the correct really keys reach the user. however, this could cause troubles when things get out of order through the backoff algorithm.

so 1. may result in completely missed out gossiped-keys while 2. may result in gossiped-keys reaching the recipient too late. i'd say the second is better :) @hpk42 have i missed something?

maybe we can also mitigate the backoff-problems by calling dc_reset_gossiped_timestamp() when a message that fails sending contains gossiped keys. or, of course, we can change the backoff-algo, however, in general these things are working in a way and maybe we should not touch this just now.

VP- commented 5 years ago

OK, I've implemented option 2.

Now, there is just one design decision I'm not sure about: the current implementation looks up the chat ID in the DB to send the DC_EVENT_MSG_DELIVERED event. If the message was deleted in the mean time, 0 is used as chat ID. The alternatives I see are:

  1. use 0 as chat ID (current implementation),
  2. not send the event for deleted messages, or
  3. store the chat ID as a param of the job (also avoids the DB query).

Any preferences here?

hpk42 commented 5 years ago

also guess option 2. is better/easier for now.

if i may add: guess it's wise if it's API-wise possible to re-do the outgoing mime message (if not sent yet). This would provide the freedom to redo encryptions+gossiping during expontential backoff etc. doesn't need to be implemented right now, though.

looking forward to seeing early-mime generation working! ;)

On Tue, Mar 05, 2019 at 02:38 -0800, björn petersen wrote:

hm, i think, this is not easy to answer.

on key-changes or changes of the memberlist, dc_reset_gossiped_timestamp() is called which signals the mime-factory that keys of group members should be re-gossiped. once done, dc_set_gossiped_timestamp() is called.

we should also keep in mind that the current exponential backoff algorithm may result in messages being sent send out-of-order (messages may also arrive out-of-order, for other reasons, of course).

  1. if we set the timestamp on SMTP-sending, the actually sent mime-structure may be created with an old set of gossiped keys, and may not include the changes that were the reasons for dc_reset_gossiped_timestamp() being called.

  2. setting the timestamp on mime-generation gives more guarantee that the correct really keys reach the user. however, this could cause troubles when things get out of order through the backoff algorithm.

so 1. may result in completely missed out gossiped-keys while 2. may result in gossiped-keys reaching the recipient too late. i'd say the second is better :) @hpk42 have i missed something?

maybe we can also mitigate the backoff-problems by calling dc_reset_gossiped_timestamp() when a message that fails sending contains gossiped keys. or, of course, we can change the backoff-algo, however, in general these things are working in a way and maybe we should not touch this just now.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/deltachat/deltachat-core/issues/427#issuecomment-469630816

VP- commented 5 years ago

One more question: What about upgrades to a version with this feature? Can we

  1. simply assume there are no queued jobs?
  2. or postpone the upgrade / tell the user to wait until the job queue is empty?
  3. or should we implement both behaviors and keep it forever for the case that somebody used the previous version, and doesn't upgrade to a newer version for years?
r10s commented 5 years ago

i think we should go for 1.

however, we should take some care to upgrades and downgrades anyway, so thanks for poining this out.

#define DC_JOB_SEND_MDN_OLD          5010    // low priority ...
#define DC_JOB_SEND_MDN              5011
#define DC_JOB_SEND_MSG_TO_SMTP_OLD  5900
#define DC_JOB_SEND_MSG_TO_SMTP      5901    // ... high priority

this way, (A) on an upgrade, old, incompatible jobs are ignored and (B) on possible downgrades, the old job format won't sneak to the new jobs on a later update. (A) could also be handled by modifying the database and increase the version in dc_sqlite3.h, howver, (B) could not be handled this way easily.