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

[ellie timoney]

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’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
> =)

Yeah, no, I don't at all insist on using time(3), was just wondering
about the reasoning for not starting there (since struct timespec is
slightly fiddlier to deal with than time_t, and we only care about
seconds resolution).  And it sounds pretty sensible, thanks :)

Cheers,

ellie

On Thu, Sep 8, 2016, at 07:37 PM, Philipp Gesang wrote:
> 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
> 
> Email had 1 attachment:
> + signature.asc
>   1k (application/pgp-signature)