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 <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 > > Email had 1 attachment: > + signature.asc > 1k (application/pgp-signature)
