Hi, thanks for the review. There’s a v3 of the patch now in which I address your points.
-<| Quoting ellie timoney <el...@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
signature.asc
Description: PGP signature
