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

[Philipp Gesang]

Hi,

thanks for the review. There’s a v3 of the patch now in which I
address your points.

-<| Quoting ellie timoney <ellie at fastmail.com>, 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",
> > +               errno, strerror(errno));
> > +        return false;
> > +    }
> 
> I'm curious about the choice of struct timespec/clock_gettime() vs
> time_t/time() -- considering we're operating at seconds (or maybe
> minutes) granularity anyway?
> 
> 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
> seem likely to be a real problem in practice (a: don't do that, b: the
> clients will just reconnect in a bit anyway).

I’m not convinced that wall clock time with all the complexity it
entails is necessary here. That’s why I went with the simpler
monotonic clock.

If you insist on using time(3) instead, I can rewrite that part
accordingly. After all, I tagged this an RFC patch for a reason
=)

> And I'm having mixed feelings about CLOCK_MONOTONIC_COARSE, which Linux
> documents as "Linux-specific".  I know we have some Solaris users out
> there (because they very kindly send us patches every time us
> Linux-using developers break it for them!)

My bad, that’s indeed an unintended Linuxism. V3 uses plain
CLOCK_MONOTONIC which is POSIX.

> Also, I'm curious about the explicit %s=>strerror(errno) in the syslog
> lines, instead of just doing something like: syslog(LOG_ERR,
> "clock_gettime: %m").  Not sure if this is a case of you not knowing
> about %m, or you knowing something I don't about %m...

Fixed.

> > +            if (deadline_exceeded(&deadline)) {
> > +                shutdown = shutdown_silent;
> > +                goto done_shutdown;
> > +            }
> 
> I notice this bit is explicitly skipping past terminating IDLE on the
> backend? I guess we specifically didn't receive an idle termination from
> the client in this case, so passing one on to the backend would be
> misleading, but I wonder.

Since no DONE arrived from the idler, I opted not to emit one on
the remote line either. But I agree it makes more sense to
cleanly leave the idle state; v3 reflects that.

> Anyway, the patch applies cleanly, builds fine for me, and passes our
> current tests.  I'll try and put together a couple of Cassandane tests
> for the new behaviour and see what shakes out of that.

Great.

Also changed with v3: When the idle timeout strikes, a debug
message is logged. This feedback makes sense considering that
it’s mainly due to buggy clients that the facility is needed at
all. Also, I added a line to reset errno that I missed when
forward porting the patch from our ancient internal cyrus tree ;)

Best,
Philipp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.andrew.cmu.edu/pipermail/cyrus-devel/attachments/20160908/dc53f982/attachment-0001.sig>