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

2016-09-26 Thread ellie timoney via Cyrus-devel
On Thu, Sep 22, 2016, at 05:55 AM, Carson Gaspar via Cyrus-devel wrote: > > If the client's connection has dropped out, no data will ever appear on > > the socket, so select will never flag it as readable, so we will never > > try to read from it, so we will never receive the read error even though

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

2016-09-21 Thread Robert Mueller via Cyrus-devel
> Back in imapd.c, cmd_idle() only reads from the client socket if > IDLE_INPUT is flagged. Which it's not after a select timeout, so if > tcp_keepalive has detected a connection dropout, cyrus still doesn't > know about it. So tcp_keepalive and select() timeout are unrelated. It appears the poi

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

2016-09-21 Thread Carson Gaspar via Cyrus-devel
On 9/19/2016 10:02 PM, ellie timoney via Cyrus-devel wrote: I've been looking at tcp_keepalive a bit lately and I'm wondering how it interacts with this? It's my understanding that, in most cases, tcp_keepalive will do the job of detecting clients that have dropped out, and allow us to close t

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

2016-09-21 Thread Andy Dorman via Cyrus-devel
On 09/20/2016 01:20 PM, Andy Dorman wrote: Ellie, I agree a "crack" exists that idled processes may be slipping through but so far I have little data to prove it. Empirically I have one server with two clients (I have moved everyone else to other servers to decrease the number of variables), and

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

2016-09-20 Thread ellie timoney via Cyrus-devel
On Tue, Sep 20, 2016, at 06:24 PM, Philipp Gesang wrote: > Another reason is to preserve the old behavior by default. It > might hit other users if the effect of the “timeout” is extended > that way. > > What about the middle ground? Cyrus could default to the > “timeout” value unless “idletimeout

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

2016-09-20 Thread ellie timoney via Cyrus-devel
On Tue, Sep 20, 2016, at 08:37 PM, Robert Mueller via Cyrus-devel wrote: > > > The thing is, during IDLE, we're not blocked on a read, we're blocked on > > a select (in idle_wait()). We don't even try to read from the client > > until the select tells us it's readable. If the client has dropped o

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

2016-09-20 Thread Andy Dorman via Cyrus-devel
On 09/20/2016 08:14 AM, Andy Dorman wrote: On 09/19/2016 09:02 PM, ellie timoney via Cyrus-devel wrote: I've been looking at tcp_keepalive a bit lately and I'm wondering how it interacts with this? It's my understanding that, in most cases, tcp_keepalive will do the job of detecting clients tha

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

2016-09-20 Thread Andy Dorman via Cyrus-devel
On 09/19/2016 09:02 PM, ellie timoney via Cyrus-devel wrote: I've been looking at tcp_keepalive a bit lately and I'm wondering how it interacts with this? It's my understanding that, in most cases, tcp_keepalive will do the job of detecting clients that have dropped out, and allow us to close th

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

2016-09-20 Thread Robert Mueller via Cyrus-devel
> The thing is, during IDLE, we're not blocked on a read, we're blocked on > a select (in idle_wait()). We don't even try to read from the client > until the select tells us it's readable. If the client has dropped out, > select never tells us it's readable, so we never try to read, so the > time

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

2016-09-20 Thread Philipp Gesang via Cyrus-devel
-<| Quoting ellie timoney , on Tuesday, 2016-09-20 14:30:22 |>- > On Tue, Sep 20, 2016, at 01:30 PM, Robert Mueller via Cyrus-devel wrote: > > Is there a reason to have a separate idle timeout separate to the > > standard inactivity timeout? > > > > timeout: 32 > > The len

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

2016-09-19 Thread ellie timoney via Cyrus-devel
On Tue, Sep 20, 2016, at 01:30 PM, Robert Mueller via Cyrus-devel wrote: > Is there a reason to have a separate idle timeout separate to the > standard inactivity timeout? > > timeout: 32 > The length of the IMAP server's inactivity autologout > timer, in minu

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

2016-09-19 Thread Robert Mueller via Cyrus-devel
> If the client's connection has dropped out, no data will ever appear on > the socket, so select will never flag it as readable, so we will never > try to read from it, so we will never receive the read error even though > tcp_keepalive detected the dropout. And if this client was idling with >

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

2016-09-19 Thread ellie timoney via Cyrus-devel
I've been looking at tcp_keepalive a bit lately and I'm wondering how it interacts with this? It's my understanding that, in most cases, tcp_keepalive will do the job of detecting clients that have dropped out, and allow us to close the connection on our end. Since we're generally either waiting

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

2016-09-14 Thread Thomas Jarosch via Cyrus-devel
Hi Ellie, On Monday, 12. September 2016 11:35:45 ellie timoney wrote: [clock jumps] > Or does it? The man page says it's "not affected by discontinuous > jumps in the system time (e.g., if the system administrator manually > changes the clock)" -- great -- "but is affected by the incremental > a

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

2016-09-11 Thread ellie timoney via Cyrus-devel
> time jumps issued via NTP are a recurring source of trouble. > On our distribution we have detection logic for that and restart > quite a few services if it happens. The monotonic clock avoids that > nicely. Or does it? The man page says it's "not affected by discontinuous jumps in the system

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

2016-09-11 Thread ellie timoney via Cyrus-devel
On Thu, Sep 8, 2016, at 07:37 PM, Philipp Gesang wrote: > thanks for the review. There’s a v3 of the patch now in which I > address your points. Looks good, builds fine, passes tests (including the new Cassandane tests for this behaviour: https://git.io/viEmd ). I'm quite happy to merge this. > I

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

2016-09-08 Thread Thomas Jarosch via Cyrus-devel
Hi Ellie, On Thursday, 8. September 2016 11:07:54 ellie timoney via Cyrus-devel wrote: > I can kind of see a point about the ability to request a monotonic > clock, such that some sysadmin changing the system time doesn't > prematurely timeout a bunch of idle connections. Though that doesn't > se

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

2016-09-08 Thread Philipp Gesang via Cyrus-devel
Hi, thanks for the review. There’s a v3 of the patch now in which I address your points. -<| Quoting ellie timoney , on Thursday, 2016-09-08 11:07:54 AM |>- > > +if (clock_gettime(CLOCK_MONOTONIC_COARSE, &now) == -1) { > > +syslog(LOG_ERR, "clock_gettime (%d %s): error reading clock"

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

2016-09-07 Thread ellie timoney via Cyrus-devel
> +if (clock_gettime(CLOCK_MONOTONIC_COARSE, &now) == -1) { > +syslog(LOG_ERR, "clock_gettime (%d %s): error reading clock", > + errno, strerror(errno)); > +return false; > +} I'm curious about the choice of struct timespec/clock_gettime() vs time_t/time() --