Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[Thomas Jarosch]

Hi ellie,

On Monday, 15 May 2017 08:32:30 CEST ellie timoney wrote:
> Planning to cut a release straight from the 2.4 git HEAD at time of
> writing (bfc80b3).  The release tarball is already built and uploaded at
> https://cyrusimap.org/releases/cyrus-imapd-2.4.19.tar.gz
> 
> If you could give that a run through your test suite, that'd be amazing.
>  If it works okay, I'll push the tag upstream, send out the formal
> announcement, and we're done.  There's a couple of patches that weren't
> there last week, but mostly it's the same as what's been there for ages.

first quick test ran fine. IMAP idle timeout patch was also
identical with what has landed in the git repo.

I've now started the full testsuite which takes around 12 hours to finish 
(tests a lot of stuff from cyrus to IPSec, horde + php etc).

Hopefully the tests will run fine since I'm not sure I'll have an Internet 
connection at home to check on the progress. Switched providers today...

Cheers,
Thomas

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[ellie timoney]

Hi Thomas,

Planning to cut a release straight from the 2.4 git HEAD at time of
writing (bfc80b3).  The release tarball is already built and uploaded at
https://cyrusimap.org/releases/cyrus-imapd-2.4.19.tar.gz

If you could give that a run through your test suite, that'd be amazing.
 If it works okay, I'll push the tag upstream, send out the formal
announcement, and we're done.  There's a couple of patches that weren't
there last week, but mostly it's the same as what's been there for ages.

Thanks,

ellie

On Tue, May 9, 2017, at 05:43 PM, Thomas Jarosch wrote:
> Hi Ellie,
> 
> On Tuesday, 09 May 2017 03:15:07 CEST ellie timoney wrote:
> > I'm looking at doing a new release of 2.4 soon -- there's been a number
> > of issues coming up lately which are already fixed in git, but aren't in
> > a released version.  It might save some support effort if these fixes
> > were released -- though it is a somewhat substantial change, what with
> > the refactored idled.
> > 
> > Are you still running 2.4 there?  Do you have any other local
> > patches/backports that have been necessary to get the refactored idled
> > stable? Would appreciate those too, if so :)
> 
> yes, still running 2.4. So far no other patches needed,
> the idled stuff has been stable since September 2016.
> 
> We have an automated testsuite for our whole distribution, so once
> there's
> a new 2.4 release candidate, we could try to plunge it in and give it a
> go.
> 
> Do you want to cut a release straight from 2.4 git HEAD?
> Or apply some other patches first?
> 
> Cheers,
> Thomas
> 

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[Thomas Jarosch]

Hi Ellie,

On Tuesday, 09 May 2017 03:15:07 CEST ellie timoney wrote:
> I'm looking at doing a new release of 2.4 soon -- there's been a number
> of issues coming up lately which are already fixed in git, but aren't in
> a released version.  It might save some support effort if these fixes
> were released -- though it is a somewhat substantial change, what with
> the refactored idled.
> 
> Are you still running 2.4 there?  Do you have any other local
> patches/backports that have been necessary to get the refactored idled
> stable? Would appreciate those too, if so :)

yes, still running 2.4. So far no other patches needed,
the idled stuff has been stable since September 2016.

We have an automated testsuite for our whole distribution, so once there's
a new 2.4 release candidate, we could try to plunge it in and give it a go.

Do you want to cut a release straight from 2.4 git HEAD?
Or apply some other patches first?

Cheers,
Thomas

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[ellie timoney]

Hi Thomas, Philipp,

On Tue, Oct 4, 2016, at 05:19 PM, Thomas Jarosch wrote:
> we also have a version of this patch for cyrus 2.4 in production now.
> If you're interested, we can upstream it, too.

Sorry for the late reply, but this would be great!

I'm looking at doing a new release of 2.4 soon -- there's been a number
of issues coming up lately which are already fixed in git, but aren't in
a released version.  It might save some support effort if these fixes
were released -- though it is a somewhat substantial change, what with
the refactored idled.

Are you still running 2.4 there?  Do you have any other local
patches/backports that have been necessary to get the refactored idled
stable? Would appreciate those too, if so :)

Thanks,

ellie

Re: Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[Thomas Jarosch via Cyrus-devel]

Hi Ellie,

On Tuesday, 4. October 2016 11:02:39 ellie timoney wrote:
> This version of the patch is now on master.  Thanks very much for
> putting it together :)

nice :) Thanks.

> I've opened https://github.com/cyrusimap/cyrus-imapd/issues/42 for
> backporting it to 2.5 -- expecting to do it soon, but making sure it
> doesn't get forgotten if I get distracted!

we also have a version of this patch for cyrus 2.4 in production now.
If you're interested, we can upstream it, too.

Cheers,
Thomas

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[ellie timoney via Cyrus-devel]

This version of the patch is now on master.  Thanks very much for
putting it together :)

I've opened https://github.com/cyrusimap/cyrus-imapd/issues/42 for
backporting it to 2.5 -- expecting to do it soon, but making sure it
doesn't get forgotten if I get distracted!

Cheers,

ellie

On Thu, Sep 22, 2016, at 02:26 AM, Philipp Gesang wrote:
> Use the value of the configuration variable “timeout” as an upper
> limit in minutes for idle connections. To allow further
> customization, add a new configuration option “imapidletimeout”
> which, if greater than zero, will be used instead. The value
> defaults to zero (not set).
> 
> RFC 2177 recommends that a client re-issue the IDLE command at
> least every 29 minutes if it wishes to continue, otherwise the
> server is free to treat the client as disconnected.
> 
> The rationale is that sometimes connections aren’t properly
> reset. Currently, a connection is not collected if it was in IDLE
> state at that point. If this happens repeatedly, imapd keeps
> accumulating dead connections which can cause DOS. This patch
> solves the problem by forcing imapd to stop idling after
> exceeding the configured timeout.
> ---
>  imap/imapd.c| 73
>  ++---
>  lib/imapoptions |  5 
>  2 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/imap/imapd.c b/imap/imapd.c
> index e2bd237..745110c 100644
> --- a/imap/imapd.c
> +++ b/imap/imapd.c
> @@ -58,6 +58,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  
>  #include 
>  
> @@ -3055,6 +3058,25 @@ static void cmd_id(char *tag)
>  imapd_id.did_id = 1;
>  }
>  
> +static bool deadline_exceeded(const struct timespec *d)
> +{
> +struct timespec now;
> +
> +if (d->tv_sec <= 0) {
> +/* No deadline configured */
> +return false;
> +}
> +
> +errno = 0;
> +if (clock_gettime(CLOCK_MONOTONIC, ) == -1) {
> +syslog(LOG_ERR, "clock_gettime (%d %m): error reading clock",
> errno);
> +return false;
> +}
> +
> +return now.tv_sec > d->tv_sec ||
> +   (now.tv_sec == d->tv_sec && now.tv_nsec > d->tv_nsec);
> +}
> +
>  /*
>   * Perform an IDLE command
>   */
> @@ -3064,6 +3086,26 @@ static void cmd_idle(char *tag)
>  int flags;
>  static struct buf arg;
>  static int idle_period = -1;
> +static time_t idle_timeout = -1;
> +struct timespec deadline = { 0, 0 };
> +
> +if (idle_timeout == -1) {
> +idle_timeout = config_getint(IMAPOPT_IMAPIDLETIMEOUT);
> +if (idle_timeout <= 0) {
> +idle_timeout = config_getint(IMAPOPT_TIMEOUT);
> +}
> +idle_timeout *= 60; /* unit is minutes */
> +}
> +
> +if (idle_timeout > 0) {
> +errno = 0;
> +if (clock_gettime(CLOCK_MONOTONIC, ) == -1) {
> +syslog(LOG_ERR, "clock_gettime (%d %m): error reading
> clock",
> +   errno);
> +} else {
> +deadline.tv_sec += idle_timeout;
> +}
> +}
>  
>  if (!backend_current) {  /* Local mailbox */
>  
> @@ -3080,6 +3122,13 @@ static void cmd_idle(char *tag)
>  
>  index_release(imapd_index);
>  while ((flags = idle_wait(imapd_in->fd))) {
> +if (deadline_exceeded()) {
> +syslog(LOG_DEBUG, "timeout for user '%s' while idling",
> +   imapd_userid);
> +shut_down(0);
> +break;
> +}
> +
>  if (flags & IDLE_INPUT) {
>  /* Get continuation data */
>  c = getword(imapd_in, );
> @@ -3112,7 +3161,8 @@ static void cmd_idle(char *tag)
>  idle_stop(index_mboxname(imapd_index));
>  }
>  else {  /* Remote mailbox */
> -int done = 0, shutdown = 0;
> +int done = 0;
> +enum { shutdown_skip, shutdown_bye, shutdown_silent } shutdown =
> shutdown_skip;
>  char buf[2048];
>  
>  /* get polling period */
> @@ -3144,6 +3194,14 @@ static void cmd_idle(char *tag)
>  
>  /* Pipe updates to client while waiting for end of command */
>  while (!done) {
> +if (deadline_exceeded()) {
> +syslog(LOG_DEBUG,
> +   "timeout for user '%s' while idling on remote
> mailbox",
> +   imapd_userid);
> +shutdown = shutdown_silent;
> +goto done;
> +}
> +
>  /* Flush any buffered output */
>  prot_flush(imapd_out);
>  
> @@ -3152,7 +3210,8 @@ static void cmd_idle(char *tag)
>  (shutdown_file(buf, sizeof(buf)) ||
>   (imapd_userid &&
>userdeny(imapd_userid, config_ident, buf,
>sizeof(buf) {
> -shutdown = done = 1;
> +done = 1;
> +shutdown = shutdown_bye;
>  goto done;
>  }
>  
> @@ -3176,12 +3235,20 @@ 

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[ellie timoney via Cyrus-devel]

Hi Andy,

At this stage the proposed patch only applies to master, whereas in your
thread on info-cyrus you mention you're on 2.5.9.

I do consider this patch -- once landed on master -- to be a possible
candidate for back porting to 2.5, but we're not at that stage yet.

Can we please continue the analysis of your situation in your thread,
and let this thread be about the patch?

Thanks,

ellie

On Wed, Sep 28, 2016, at 01:01 AM, Andy Dorman via Cyrus-devel wrote:
> Don't know if my vote counts, but we are still accumulating over 100 
> idled connections every 24 hours from two accounts being checked by an 
> old (+5 yrs) BlackBerry.
> 
> So I believe an idled timeout could help us there.
> 
> Due to my workload I have not been able to do any network sniffing on 
> the Blackberry connections to irrefutably confirm the cause of the 
> problem, but I wouldn't know what to look for anyway. I am not going to 
> give up capturing the packets though.  At the very least I can probably 
> learn more about IMAP and that is a good thing.
> 
> Andy
> 
> On 09/27/2016 01:07 AM, ellie timoney via Cyrus-devel wrote:
> > On Thu, Sep 22, 2016, at 01:29 AM, Philipp Gesang wrote:
> >> Minutes again, because IMO a fallback between different units of
> >> measure would be confusing.
> >
> > Yeah, I was thinking the same thing. :)  The updated patch looks good to
> > me.
> >
> > So it looks like what we're getting from this is two things:
> >
> > * making the "timeout" limit apply to idling clients
> > * optionally configuring a separate, longer limit specifically for
> > idling clients
> >
> > The former restores old behaviour that looks like it was accidentally
> > lost during the conversion to use a socket rather than signals for idled
> > IPC.
> >
> > The latter then allows administrators to be more lenient with clients
> > that are idling, even if they aren't respecting the
> > restart-after-29-minutes convention.
> >
> > Both of those seem good to me.
> >
> > And they will both also protect Cyrus from the possibility of idling
> > connections dropping out and never being cleaned up properly (though
> > if/when/how this occurs is still subject to speculation).
> >
> > Cheers,
> >
> > ellie
> >
> 

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[Andy Dorman via Cyrus-devel]

Don't know if my vote counts, but we are still accumulating over 100 
idled connections every 24 hours from two accounts being checked by an 
old (+5 yrs) BlackBerry.


So I believe an idled timeout could help us there.

Due to my workload I have not been able to do any network sniffing on 
the Blackberry connections to irrefutably confirm the cause of the 
problem, but I wouldn't know what to look for anyway. I am not going to 
give up capturing the packets though.  At the very least I can probably 
learn more about IMAP and that is a good thing.


Andy

On 09/27/2016 01:07 AM, ellie timoney via Cyrus-devel wrote:

On Thu, Sep 22, 2016, at 01:29 AM, Philipp Gesang wrote:

Minutes again, because IMO a fallback between different units of
measure would be confusing.


Yeah, I was thinking the same thing. :)  The updated patch looks good to
me.

So it looks like what we're getting from this is two things:

* making the "timeout" limit apply to idling clients
* optionally configuring a separate, longer limit specifically for
idling clients

The former restores old behaviour that looks like it was accidentally
lost during the conversion to use a socket rather than signals for idled
IPC.

The latter then allows administrators to be more lenient with clients
that are idling, even if they aren't respecting the
restart-after-29-minutes convention.

Both of those seem good to me.

And they will both also protect Cyrus from the possibility of idling
connections dropping out and never being cleaned up properly (though
if/when/how this occurs is still subject to speculation).

Cheers,

ellie

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[ellie timoney via Cyrus-devel]

On Thu, Sep 22, 2016, at 01:29 AM, Philipp Gesang wrote:
> Minutes again, because IMO a fallback between different units of
> measure would be confusing.

Yeah, I was thinking the same thing. :)  The updated patch looks good to
me.

So it looks like what we're getting from this is two things:

* making the "timeout" limit apply to idling clients
* optionally configuring a separate, longer limit specifically for
idling clients

The former restores old behaviour that looks like it was accidentally
lost during the conversion to use a socket rather than signals for idled
IPC.

The latter then allows administrators to be more lenient with clients
that are idling, even if they aren't respecting the
restart-after-29-minutes convention.

Both of those seem good to me.

And they will both also protect Cyrus from the possibility of idling
connections dropping out and never being cleaned up properly (though
if/when/how this occurs is still subject to speculation).

Cheers,

ellie

Re: [PATCH v4] imapd.c: imapoptions: implement idle timeout

[Philipp Gesang via Cyrus-devel]

-<| Quoting Philipp Gesang , on Wednesday, 
2016-09-21 17:26:40 |>-
> Use the value of the configuration variable “timeout” as an upper
> limit in minutes for idle connections.

   ^^^

Minutes again, because IMO a fallback between different units of
measure would be confusing.

Best,
Philipp



signature.asc
Description: PGP signature