[tor-bugs] #18365 [Core Tor/Tor]: Fined-grain timer implementation to support per-connection or per-circuit timers

[Tor Bug Tracker & Wiki]

#18365: Fined-grain timer implementation to support per-connection or per-circuit
timers
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  nickm
     Type:  enhancement                          |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  performance, backend,                |        Version:
  TorCoreTeam201604                              |     Resolution:
Parent ID:                                       |  Actual Points:  medium
 Reviewer:                                       |         Points:  medium
                                                 |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by andrea):

 * status:  needs_review => needs_revision


Comment:

 Begin code review for nickm's timeouts_v2 branch:

 32e80ea3d32d5fd8207d16f9e5b26defa0d98a7c:

  - No detailed review of this commit as this is an import of external code

  - One potential concern: we need to be pretty clear whether the times
    we use here are meant to be monotonic times or real-time clock times;
    it doesn't look like timeout.c makes any direct time syscalls itself,
    just lets the caller supply the current time as a uint64_t, which gives
    us flexibility there, but it will be necessary to be consistent within
    any given struct timeouts.  Possibly some admonitory documentation is
    in order somewhere.

    - Okay, in d0638fe760363f9c040256ac2884234ddad1d384 it looks like we're
      committing to monotonic time in libevent_timer_reschedule().
 Consider
      this concern resolved.

 05499b6ded25b5cbc8b16916fa9c0a39407ab10f:

  - Straightforward makefile changes; this looks fine

 9d6c530015e4eefd7b333885eaeca1f9fcbc6578:

  - Stop compiler warnings for timeout.c; looks okay, but are we sure we
    got everything for every possible case we end up building?

  - Same for cbf47612b737a6ad31f17084ef5c36f5ebe33a76; some nervousness
    about all these bitmasks and casts.

 c77cf8825a33d902c5827f0b4f0a71cec97a3a85:

  - This looks pretty straightforward and unobjectionable.

 d0638fe760363f9c040256ac2884234ddad1d384:

  - Is using a single pool of timeouts for both absolute and relative
    values with tor_gettimeofday_cached_monotonic() entirely wise?

    - From the comment for that function:

 {{{
      * In future versions of Tor, this may return a time does not have its
      * origin at the Unix epoch.
 }}}

      This, of course, is intended to allow for a future change to
      clock_gettime(CLOCK_MONOTONIC, ...), which will provide smoother
      behavior in the case the clock actually does run backwards than
      the current remember-and-compare implementation, but is not
 guaranteed
      to have any particular origin.  Using that where available is clearly
      the most reliable way to handle relative timeouts, but wrong for
 absolute
      ones, and the function we're using is defined to allow us room to
 make
      that change in the future.

  - Do we actually have any examples of needing to fire a particular
 callback
    exactly once at or after a particular wall clock time as these absolute
    timeouts provide?

  - If we do need absolute timeouts, we should think about splitting things
    up into two timeouts objects rather than just a unified
 global_timeouts;
    if relative timeouts are monotonic and absolute timeouts are based on
    clock time, then resetting time potentially changes the order of future
    timeouts without any timeout having occurred.  Then we're free to use
    the appropriate definition of time separately for each.

 f7a6f142c67a3d256d522d1cfa66e525d16d6ab7:

  - These tests look just fine.

 cbf47612b737a6ad31f17084ef5c36f5ebe33a76:

  - See comments for 9d6c530015e4eefd7b333885eaeca1f9fcbc6578

  - This particular cast in timeouts_del(), for example, will break
    on a machine with 64-bit pointers and 32-bit ints if WHEEL_LEN
    is very large.

    - timeout.c does document size constraints on these parameters
      such that this should work on any platform with integer sizes
      consistent with ISO C, though.

 b9e0f7c91623e5ebde774bab61030f04b808c024:

  - More tests; looks okay.

 d3a2b9e836cfed39f64963b171be152a7ae8ff4b:

  - Changes file looks fine

 In conclusion, this needs more thought about relative vs. absolute
 timeouts
 I think.  Should I review the imported timeout.c code itself separately,
 or is this something solid and widely used enough we aren't overly worried
 about it?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18365#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online