mail icon indicating copy to clipboard operation
mail copied to clipboard

Add imip processing in the background

Open miaulalala opened this issue 3 years ago • 5 comments

Fixes #6802

Questions:

what happens when I have a very large mailbox where I never trigger the preview for messages below the fold?

miaulalala avatar Jun 30 '22 12:06 miaulalala

The way we currently only load envelope data means we can never really process all messages as we don't know whether they contain an iMIP body part.

Suggestion 1: only process messages that are opened (where the loading of the body data means we would update the is_imip_field in the db and later pick it up in the background job)

Benefits: efficient, no overhead Drawbacks: we can never be sure we have processed all iMIP messages in the mailboxes and might miss important ones. Since the processing itself is done hidden in a background job it could confuse users as to why certain messages were processed and others weren't.

Suggestion 2: get all new messages in a mailbox and additionallyload their body content too in a background job (whether we extend an exisiting job or add a new one could be debatable - I would tend towards option 1).

Benefit: we would definitely catch all messages Drawback: loading data from IMAP is slow and could hit us straight into the performance.

I would prefer option 2.

For the preformance issue I propose the following solution (thanks to @st3iny who helped me on this):

We could extend the horde cache and implement our own horde cache class that lets us cache body content, or we could use ICache. It is more work but it would enable us to cache IMAP messages making Mail a lot faster. Since IMAP messages are immutable, we could cache them indefinitely or until they are deleted - or we could set a cache limit of a couple of weeks or even a month, it would still help us.

I would not cache message bodies in the database.

It would also help with full text search for mail, etc...

miaulalala avatar Aug 03 '22 10:08 miaulalala

I'm pretty strongly against 1 because it doesn't work well if you aren't using the nextcloud webmail. Users of other webmail, other IMAP clients or even just people who rarely check their email won't get these updates handled. For example if I am on my phone I don't use the nextcloud mail UI and I may want to check RSVPs to an event and have an up-to-date number without checking my mail.

So I still think 2 is best. However even that isn't perfect. For example if I get a message on my phone and quickly delete it IIUC this may not be processed since it wasn't in the inbox for any 5min background job interval. It is probably a good "baseline" option but maybe there could be an advanced option where the mailserver could post new messages to nextcloud for processing? This could be a simple filter rule to ensure that nothing is missed and that imip messages are processed in real time. However that is probably a later improvement.

kevincox avatar Aug 03 '22 11:08 kevincox

I'm pretty strongly against 1 because it doesn't work well if you aren't using the nextcloud webmail. Users of other webmail, other IMAP clients or even just people who rarely check their email won't get these updates handled. For example if I am on my phone I don't use the nextcloud mail UI and I may want to check RSVPs to an event and have an up-to-date number without checking my mail.

So I still think 2 is best. However even that isn't perfect. For example if I get a message on my phone and quickly delete it IIUC this may not be processed since it wasn't in the inbox for any 5min background job interval. It is probably a good "baseline" option but maybe there could be an advanced option where the mailserver could post new messages to nextcloud for processing? This could be a simple filter rule to ensure that nothing is missed and that imip messages are processed in real time. However that is probably a later improvement.

Thanks for your input! I do tend to think option 2 is a good solution, if imperfect. But we're also additionally processing any opened messages anyway (that part already works) - it's just the unopened, undownloaded messages we need a way to deal with.

Good point on the delete, if you open an iMIP message, it might be a great idea to process it immediately... at least for a REPLY. Something to think about.

Unfortunately, mail servers are not capable of pushing changes to external clients. That is not a part of the IMAP standard, so the only way to work with an IMAP server is to ask the server for changes.

miaulalala avatar Aug 03 '22 13:08 miaulalala

Unfortunately, mail servers are not capable of pushing changes to external clients.

There is IDLE support, but IDK how well that work work for nextcloud. Definitely doesn't fit the current architecture with no long-running processes.

Also many support filtering and other tools that can send requests to other services. Not part of IMAP, but still a useful option. As I said it would be a nice future feature and the base polling serves as a common ground that works for all IMAP servers with no extra setup.

kevincox avatar Aug 03 '22 13:08 kevincox

Unfortunately, mail servers are not capable of pushing changes to external clients.

There is IDLE support, but IDK how well that work work for nextcloud. Definitely doesn't fit the current architecture with no long-running processes.

Also many support filtering and other tools that can send requests to other services. Not part of IMAP, but still a useful option. As I said it would be a nice future feature and the base polling serves as a common ground that works for all IMAP servers with no extra setup.

taking the discussion to the linked ticket as this is supposed to be for code reviews only ;)

miaulalala avatar Aug 03 '22 18:08 miaulalala

  • [x] add another BG job that processes all messages until 2 weeks into the past for structure Analysis. (should make mail app faster)

miaulalala avatar Aug 11 '22 14:08 miaulalala

just waiting for server PR now

miaulalala avatar Aug 22 '22 20:08 miaulalala

stable22, 23 and 24 test failing is not surprising as this isn't backported

miaulalala avatar Sep 06 '22 09:09 miaulalala

@ChristophWurst I added a check on the IManager to see if the method handleImipReply exists so we can avoid the whole test suite exploding for the next however many releases.

Shamelessly stolen from https://github.com/nextcloud/calendar/blob/main/tests/php/unit/Service/Appointments/BookingServiceTest.php

miaulalala avatar Sep 07 '22 22:09 miaulalala

linter

ChristophWurst avatar Sep 08 '22 14:09 ChristophWurst