fix: Emit WebxdcInstanceDeleted events when deleting a chat (#6670) #6718
Conversation
|
nvm I forgot it is about one of the few events with side effects To protect against crashes we could alternatively save a list of message ids to delete somewhere in the db and add an api to retrieve them on appstartup and when there are WebxdcInstanceDeleted events |
i do not think, that effort is needed in practise. when the app crashes the webxdc window etc is usually also removed. might be cornercases left for notifications or icons not being removed, but that alone would not justify keeping events in database (i know, there is some incentive to do that for $reasons, but that is out of scope of this PR) in general, it should not be the end of the world when an event is missing after program start
|
|
deltachat desktop and deltachat tauri create and store different browser sessions/data directories for each account and on dc tauri even for each webxdc. So loosing the event will mean there is a folder not cleaned up until you delete the account. (webxdc's data is still on file system) @r10s I think mobile(iOS and android) just uses different origins for isolation. I don't know if mobile removes the data from a webxdc at all, after the message is deleted, so might not apply to mobile, yet. |
iequidoo
force-pushed
the
iequidoo/chat-delete-WebxdcInstanceDeleted
branch
from
a552642 to
f2c02d1
Compare
|
well, data is deleted after 30 days or so. not sure about indexdb, which however is not used much in practise. compared to all the overhead, for now, we can ignore that issue for desktop, it seems be easy enough to iterate over the dirs and delete the ones without a message. but also there, it seems fine to accept some overhead for now. remember, we're talking about app crashes at exact time of delition, better spend time to fix crashes (have we seen any crash in that area?) |
where do you get that number from?
not that I know of, it seems to be theoretical. seems like @iequidoo brought it up as potential issue. |
src/chat.rs
Outdated
| // Let's emit events optimistically and not return webxdc `MsgId`s from the | ||
| // transaction to not add LIMIT to SELECT above. |
There was a problem hiding this comment.
I don't understand the comment. what is the meaning of it?
There was a problem hiding this comment.
I tried to improve the comment. I just don't want to allocate a vector of unlimited size
| assert_eq!(msg_id, instance.id); | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
would make sense to also make a test that checks that WebxdcInstanceDeleted is only emitted for deleted webxdc messages and one that makes sure only that it only emits the events from the chat where the message was deleted from.
There was a problem hiding this comment.
I extended the test, now it creates two chats with webxdc messages and also sends a text message to the chat aimed for deletion
If so, i think that the app should first list webxdc messages in the chat, then delete all associated data, then messages themselves (all this can be done in several iterations if there are many webxdc messages) and only then delete the chat itself, i.e. the app shouldn't rely on events (which still can be emitted at least for feature-completeness). If mobile apps don't need to clean up anything additionally, they can just delete a chat as currently. |
|
don't forget that message deletion is synced between devices, we still need the event. |
iequidoo
force-pushed
the
iequidoo/chat-delete-WebxdcInstanceDeleted
branch
from
f2c02d1 to
90a840b
Compare
True, so we anyway need to add some kind of persistent events to make webxdcs deletion reliable at least for Desktop. There are other ways in which messages may be deleted, e.g. |
some ppl were complaining that eg, wonster stats are gone after not opening the app some time :) afaik, there is no "hard timelimit", though. but on "storage pressure" browser/webview seem to clean up we discussed that several times, agreeing that for now, it is fine. eg. spending our resources into deduplication probably saves 10-100 times more storage than thinking too much about localstorage/indexdb. the vast majority of cases is small data, if there is sth left, then it's that, at some point it will be cleaned 🤷♂️ and yes, i agree on ignoring the issue for now to not complicate things. |
src/chat.rs
Outdated
| // Let's emit events optimistically (before the transaction completes) and not | ||
| // return webxdc `MsgId`s from the transaction because otherwise we need a vector of | ||
| // indefinite size in memory. |
There was a problem hiding this comment.
while probably okay to emit the events before, i do not agree with the reasoning that a "a vector of indefinite size in memory" is an issue here.
i have a huge account, and if it is much in the largest chat (testing!) maybe 500 webxdc. a vector of that size, and even if it is 500x more is not an issue at all.
There was a problem hiding this comment.
I moved the code emitting events outside of the transaction and removed the comment. If the transaction becomes too huge, e.g. the chat contains too many messages, it doesn't scale well anyway and would block executing other transactions and the app, so let's not try to solve such problems in this PR
iequidoo
force-pushed
the
iequidoo/chat-delete-WebxdcInstanceDeleted
branch
from
90a840b to
5a5f2c5
Compare
Not sure that emitting events optimistically is a good solution, but emitting them after the transaction (as we do everywhere) is also not reliable if the program crashes before that. It seems that the only proper way is to introduce persistent events which are removed by the app from db after processing. Returning an indefinite number of MsgId-s from the transaction may eat up memory, but adding LIMIT to SELECT makes the fix just useless.
Fix #6670