Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#18908 closed enhancement (fixed)

Implement monotonic time in a better way than simply calling down to libevent's non-monotonic time

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-5 TorCoreTeam201607
Cc: Actual Points: 1
Parent ID: Points: 1
Reviewer: athena Sponsor:

Description

For a synopsis of the various better ways, see what libevent 2.1 will someday do, in https://github.com/libevent/libevent/blob/master/evutil_time.c#L137 .

Child Tickets

Change History (14)

comment:1 Changed 5 years ago by nickm

Points: small

comment:2 Changed 5 years ago by nickm

Keywords: 029-nickm-unsure added

Marking these tickets as the ones I think I need more feedback about in order to figure out if I think it should go in 0.2.9.

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.9.x-final

Calling these "yes for 0.2.9" since a good implementation of mike's padding patches requires them.

comment:4 Changed 5 years ago by nickm

Keywords: 029-proposed. 029-nickm-unsure removed

comment:5 Changed 4 years ago by nickm

Points: small1

comment:6 Changed 4 years ago by nickm

Owner: set to nickm
Status: newaccepted

I've started this in a branch monotonic_v2. More work is needed: I need to test it and finish the windows code.

comment:7 Changed 4 years ago by nickm

Keywords: review-group-5 added

comment:8 Changed 4 years ago by nickm

Reviewer: athena

comment:9 Changed 4 years ago by nickm

Actual Points: 1
Status: acceptedneeds_review

Now with unit tests and a changes file

comment:10 Changed 4 years ago by andrea

Begin code review for nickm/monotonic_v2:

Summary: I think I'm good with this being merged as long as all four cases
(clock_gettime(), gettimeofday(), Windows and Mac OS) got tested.

aa971c59246332391116caafd18855bccf2f99a5:

  • Just moving code here. Looks good to me.

e42e054fbfb5dd8d4a5729f9f7988b2def60840e:

  • Good, this doesn't make any particular assumptions about the origin of monotonic time. On current Linux, CLOCK_MONOTONIC is actually since system boot. The parts for platforms I know look okay to me, but did we manage to actually test the Windows / Mac OS X stuff?

(cf. e431ed37360a405f36e07a8c902146f0b4f4b31b)

a2e5704a912ba06255c56de5603fce6444ebad98:

  • Looks fine.

4baf1d04db6c8e84a113cc801284be3dbe23884e:

  • Looks okay.

e431ed37360a405f36e07a8c902146f0b4f4b31b:

  • Hmm, looks okay but have we tested it on Windows yet?

8edc842182df6c2036e51d5e4c3de6248f3065f9:

  • Just test suite exposure.

1e21947464f81fb55e6f6ce52c43244933ef6657:

  • Okay, it compiles but have we tested it? :P

cb45ee9494c93064ba61f6503aa9abfd583555db:

  • Woot, a pure code deletion commit

81ca7a984a5a7820a5af5b9f1ad1f59d8c0ebc7f:

  • Looks fine.

49ec146a3c7876c03040fefcab6345146d3b9ef3:

  • Unit tests only in this one; they look fine.

8f0ba05a2522c56eebb8d239c5a3f431e88a7261:

  • Just the changes file

End code review

comment:11 Changed 4 years ago by nickm

Status: needs_reviewmerge_ready

Your main questions were about testing. I ran the tests on Linux, OSX, and on Linux with clock_gettime artificially disabled. They all seemed to work okay. I compiled the windows tests, but didn't have an easy way to run them. I think I'll just merge, though, and see what Jenkins thinks? Unless you object.

comment:12 Changed 4 years ago by andrea

Yeah, I think I'm happy with that.

comment:13 Changed 4 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay. Squashed and merged.

comment:14 Changed 4 years ago by nickm

Keywords: TorCoreTeam201607 added
Note: See TracTickets for help on using tickets.