Opened 2 years ago

Closed 15 months ago

#25113 closed defect (fixed)

monotonic_time unit test fail, debian armel

Reported by: arma Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: tor-test, 029-backport, 031-unreached-backport, 032-unreached-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


<weasel> util/monotonic_time:
<weasel>   FAIL ../src/test/test_util.c:5843: assert(msecc1 OP_LE nsecc1 / 1000000 + 1): 41399 vs 41395
<weasel>   [monotonic_time FAILED]

Looks like the armel build failed:

The comment in the unit tests right above that is

  /* We need to be a little careful here since we don't know the system load.

which makes me suspicious. :)

Child Tickets

Change History (13)

comment:1 Changed 2 years ago by dgoulet

Keywords: tor-test added
Status: newneeds_information

Wow ok so there is literally a 4msec gap between the nsecc1 and msecc1 function call on armel.

  nsecc1 = monotime_coarse_absolute_nsec();
  [usecc1 = monotime_coarse_absolute_usec();]
  msecc1 = monotime_coarse_absolute_msec();

So this test is doomed to fail on slow system because we get nsecc1 before msecc1 so shouldn't be msecc1 >= nsecc1 instead?

  tt_u64_op(msecc1, OP_LE, nsecc1 / 1000000 + 1);
  -> msecc1 <= nsecc1

Actually the whole set of checks between the 1 variables seems a bit weird, it assumes the clock gettime calls to be in the same msec range.

Also, clock_gettime() is a vdso in ARM exported since Linux 4.1 so if the build machine has an older kernel, the time gaps can be bigger.

comment:2 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: needs_informationaccepted

Quick discussion with nickm on IRC, bumping to 10msec is most likely what we want because we need to check for the synchronization between time read. Not ideal fix but for now that should do it.

If we ever encounter a system that will have a 10msec+ gap between the function call, we'll adapt at that point.

Branch: bug25113_033_01

comment:3 Changed 2 years ago by dgoulet

Status: acceptedmerge_ready

Kind of trivial fix so merge_ready.

comment:4 Changed 2 years ago by dgoulet

Keywords: 029-backport 031-backport 032-backport added

Oh and the 029 branch for backport: bug25113_029_01

comment:5 Changed 2 years ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.2.x-final

Merged to master; marking as backportable.

comment:6 Changed 2 years ago by teor

Keywords: 031-unreached-backport added; 031-backport removed

0.3.1 is end of life, there are no more backports.
Tagging with 031-unreached-backport instead.

comment:7 Changed 19 months ago by teor

Keywords: 032-unreached-backport added; 032-backport removed

0.3.2 is end of life, so 032-backport is now 032-unreached-backport.

comment:8 Changed 19 months ago by teor

Version: Tor:

comment:9 Changed 19 months ago by teor

Version: Tor:

comment:10 Changed 19 months ago by teor

Milestone: Tor: 0.3.2.x-finalTor: 0.2.9.x-final

These tickets can't be backported to 0.3.2, because it is end of life.
But they can still be backported to 0.2.9.

comment:11 Changed 15 months ago by teor

This is a simple test-only change that makes the unit tests more reliable.
It is a backport candidate.

But since the branch is old, I am opening a pull request to check CI still passes:

comment:12 Changed 15 months ago by teor

Version: Tor:

comment:13 Changed 15 months ago by teor

Resolution: fixed
Status: merge_readyclosed

I merged #25116, #25113, and #24903 into 0.2.9.

CI passed on:

  • the original branches,
  • each branch merged into the current maint-0.2.9, and
  • all 3 branches merged together into maint-0.2.9, then merged into release-0.2.9.
Note: See TracTickets for help on using tickets.