Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

storage: remove 500 ms debounce on IMAP IDLE notifications #216

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

link2xt
Copy link

@link2xt link2xt commented Jan 11, 2024

This commit removes hardcoded 500 ms debounce
from storage that delays all storage notification subscribers such as IDLE and NOTIFY commands.

500 ms debounce constant NOTIFY_DELAY_MSECS
was added in 2009 [1]. Before that Dovecot
was only delivering notifications
when a second-resolution timer
is changed by at least 1, so IDLE notifications
were delayed by half a second on average.

[1] 56fb5d0

This commit removes hardcoded 500 ms debounce
from storage that delays all storage notification subscribers
such as IDLE and NOTIFY commands.

500 ms debounce constant NOTIFY_DELAY_MSECS
was added in 2009 [1]. Before that Dovecot
was only delivering notifications
when a second-resolution timer
is changed by at least 1, so IDLE notifications
were delayed by half a second on average.

[1] dovecot@56fb5d0
@link2xt
Copy link
Author

link2xt commented Jan 11, 2024

I have made a patch that actually deletes 500 ms delay
from mailbox-watch.c rather than reducing it to 1 ms.

I then looked into
adding debounce to src/imap/cmd-idle.c,
but there I cannot be sure that mailbox pointer
passed to idle_callback() will not be freed
by the time debounce timer expires.
If we want to have debounce in IDLE,
it should be kept in mailbox-watch.c,
but reset via special API from cmd-idle.c
every time IDLE is started.

Then I thought about the issue again
and am pretty sure debounce is not needed for IDLE.
If an offline client gets online and synchronizes
a lot of flags, it usually can do it in one command.
If it does not, because of bad implementation
or because flags are different for different messages,
it will send multiple commands.
In this case IDLE-ing client will receive
multiple updates, but this is not bad for performance.
If IDLE-ing client does not ignore these updates
and actually processes them,
worst case it will immediately get busy
processing the first incoming update.
But by the time it finishes processing
the first update it will receive many updates
and process them in batch,
effectively debouncing on the client
even if there is no special code doing this.

So unless there is an IMAP client
that is known to benefits from debouncing,
I suggest that no debouncing is added for IDLE.

See related issue at chatmail/server#72 for the "chatmail" Postfix+Dovecot setup that needs reduced delay for chatting.
There is a related thread on the Dovecot mailing list:
https://dovecot.org/mailman3/archives/list/dovecot@dovecot.org/thread/J2L67F75QW5MJBIRKMBGA2AKNJHRC33X/

@link2xt
Copy link
Author

link2xt commented Jan 17, 2024

This is now deployed on https://c20.testrun.org/ account and works without problems.

@sirainen
Copy link
Contributor

The worry I have related to performance isn't about IMAP client behavior, but rather Dovecot's mailbox_sync() calls. With no delay IDLEing imap process would call mailbox_sync() every time there is a write to dovecot.index.log, whatever that reason is. If another sync is already running by another process, it would wait a bit for the exclusive lock, but otherwise it would be syncing as rapidly as there are .log file writes. Although syncing isn't necessarily expensive, it's not trivially cheap either. So this might increase the server's CPU usage. I'm not sure if that's more of a theoretical problem, but over the years I've grown to be wary of performance regressions, since they sometimes pop up rather unexpectedly.

@link2xt
Copy link
Author

link2xt commented Jan 31, 2024

Without measuring I also cannot tell
how much it affects performance
and have no idea how to benchmark Dovecot myself.

An option that was discussed in the mailing list
was to notify immediately if started IDLE has not notified anything yet
and add new APIs to mailbox-watch to reset the timer from IDLE implementation.
I do not like this option because it would not work for NOTIFY extension
which is only started once.

I have another proposal.
Would it be possible to reset the debounce timer every time a
MessageNew
event is received?
This will ensure that MUAs are notified about incoming messages immediately,
but other changes to maildir or other storage like setting flags
are still debounced.
As long as the inbox is not constantly flooded with incoming messages,
nothing will change for the server load.
I have not looked in detail how to implement it,
but imagine it should be possible
to do the same as the notify plugin
does to subscribe to "mail_save" events.

If nothing else works,
we can still resort to adding a configuration option for this delay
so we can at least configure it to 0.
But most servers will not change the default though
and have 500 ms delay, which is unfortunate,
I would prefer to solve this for everyone
so eventually all servers that are updated will have no delay.

@missytake
Copy link

If anyone wants to try this out, we are hosting this PR as a .deb build on https://build.opensuse.org/package/show/home:deltachat/dovecot so it is easy to install

@nimbius
Copy link

nimbius commented Feb 5, 2025

@sirainen with respect to the CPU concern in 2009 I believe this would be cause to reject the merge; cost is a real factor at scale.

In 2025 I dont believe its a concern. The smallest mailserver is still equipped with 4-8 CPU cores. Scalable IMAP servers are all wildly more performant than this however. Epyc or Intel, I think the concern is moot

@dkg
Copy link

dkg commented Feb 21, 2025

I've asked for this change in debian's experimental repository, and the maintainer there is looking for more feedback from upstream on this change. It would be great to understand if this patch is under consideration.

Thanks for dovecot, it has been a dependable workhorse for me for years!

@link2xt
Copy link
Author

link2xt commented Feb 21, 2025

dovecot deliberately delays all NOTIFY and IDLE by 500ms. It's not clear why it does that.

It's not exactly a delay, but a debounce, so if two IDLE notifications arrive within 500 ms, callback is called only once. This does not happen normally, maybe if you move a lot of messages from one folder to another or use https://imapsync.lamiral.info/ while having a client open, but then that's it, you will get an IDLE interruption on every message rather than every 500 ms, it's anyway should not be more expensive than actually writing a mail to the folder.

On a chatmail server which delivers around 3-5 mails per second (with deduplication etc., so some of these mails are hardlinks, but still) this does not cause any problems so far.

@sirainen
Copy link
Contributor

How about something like this, which starts with 0 delay but increases it for a while and periodically resets it back to 0? Could still be tweaked a bit I guess, but I'd think this would work well enough for your use case while guaranteeing that there's no huge flooding of the notify callback:

diff --git a/src/lib-storage/mail-storage-private.h b/src/lib-storage/mail-storage-private.h
index 6732bd80e4..3a1dcd4a97 100644
--- a/src/lib-storage/mail-storage-private.h
+++ b/src/lib-storage/mail-storage-private.h
@@ -458,6 +458,13 @@ struct mailbox {
 	struct timeout *to_notify, *to_notify_delay;
 	struct mailbox_notify_file *notify_files;
 
+	/* Next timestamp when notifications can be immediately sent without
+	   delay. */
+	struct timeval notify_delay_reset_time;
+	/* Number of notifications since notify_delay_reset_time was updated.
+	   This is used to calculate an increasing delay. */
+	unsigned int notify_delay_counter;
+
 	/* Increased by one for each new struct mailbox. */
 	unsigned int generation_sequence;
 
diff --git a/src/lib-storage/mailbox-watch.c b/src/lib-storage/mailbox-watch.c
index 659cab3810..bad8bbb5f7 100644
--- a/src/lib-storage/mailbox-watch.c
+++ b/src/lib-storage/mailbox-watch.c
@@ -2,6 +2,7 @@
 
 #include "lib.h"
 #include "ioloop.h"
+#include "time-util.h"
 #include "mail-storage-private.h"
 #include "mailbox-watch.h"
 
@@ -9,7 +10,7 @@
 #include <fcntl.h>
 #include <sys/stat.h>
 
-#define NOTIFY_DELAY_MSECS 500
+#define NOTIFY_DELAY_MAX_MSECS 500
 
 struct mailbox_notify_file {
 	struct mailbox_notify_file *next;
@@ -45,13 +46,27 @@ static void notify_timeout(struct mailbox *box)
 
 static void notify_callback(struct mailbox *box)
 {
-	timeout_reset(box->to_notify);
-
-	if (box->to_notify_delay == NULL) {
-		box->to_notify_delay =
-			timeout_add_short(NOTIFY_DELAY_MSECS,
-					  notify_delay_callback, box);
+	unsigned int timeout_msecs = 0;
+
+	if (box->to_notify_delay != NULL)
+		return;
+
+	/* The first notification is immediate. The next notification is
+	   delayed by 10 ms, then 20 ms, 40 ms, .. up to NOTIFY_DELAY_MAX_MSECS.
+	   After NOTIFY_DELAY_MAX_MSECS has passed since the initial delay,
+	   the delay timer is reset. */
+	if (timeval_cmp(&ioloop_timeval, &box->notify_delay_reset_time) < 0) {
+		timeout_msecs = 10U << box->notify_delay_counter++;
+		if (timeout_msecs > NOTIFY_DELAY_MAX_MSECS)
+			timeout_msecs = NOTIFY_DELAY_MAX_MSECS;
+	} else {
+		box->notify_delay_counter = 0;
+		box->notify_delay_reset_time = ioloop_timeval;
+		timeval_add_msecs(&box->notify_delay_reset_time,
+				  NOTIFY_DELAY_MAX_MSECS);
 	}
+	box->to_notify_delay =
+		timeout_add_short(timeout_msecs, notify_delay_callback, box);
 }
 
 void mailbox_watch_add(struct mailbox *box, const char *path)

@link2xt
Copy link
Author

link2xt commented Feb 21, 2025

@sirainen This approach has a non-continuous behavior that is not easy to analyze: if email arrives exactly every 10.1 ms then there is no delay, but if email arrives every 9.9 ms, it will be delayed by half a second eventually. Having a sudden change in resource usage when the load crosses 1 message per 10 ms threshold looks strange.

In the case you want to keep debouncing, I'd go for GCRA (leaky bucket) ratelimiting, say allow 1 notification per 500 ms, but with a burst of 5 notifications. Then if you have a continuous stream of messages from e.g. imapsync, it immediately causes 5 notifications and then one notification per 500 ms, while for normal email usage (or chatting over email) there are no delays ever. If you increase the rate of incoming messages from 0 to 2 messages per second, the rate of notifications grows linearly, then stays at 2 notifications per second.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants