Opened 5 months ago

Closed 5 months ago

#25994 closed defect (fixed)

test_channel: keep time constant when running channel/outbound_cell

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci, tor-tests-coverage, tor-tests-unit
Cc: Actual Points:
Parent ID: #25908 Points:
Reviewer: mikeperry Sponsor: Sponsor3-can

Description

With fairly low probability, the outbound_cell test could get run with cached_gettimeofday on a different 10-second boundary than the current value of gettimeofday, resulting in a call to scale_active_circuits().

Now that we no longer use cached_gettimeofday, the probability of calling scale_active_circuits is even lower here. If the test takes about 200 microseconds (as it does on my desktop), we should only expect it to straddle the 10-second boundary with P < 200 usec / 10s == 1 / 50000.

Still, it's a good idea to fix this kind of thing.

This change will _lower_ coverage from the maximum by making it so this test does not call scale_active_circuits. However, this test does not actually validate any of the behavior of scale_active_circuits, so this change is appropriate.

Child Tickets

Change History (6)

comment:1 Changed 5 months ago by nickm

Status: assignedneeds_review

https://github.com/torproject/tor/pull/73 is a pull request here; the branch is bug25994.

comment:2 Changed 5 months ago by nickm

Parent ID: #25993#25908

comment:3 Changed 5 months ago by nickm

Sponsor: Sponsor3-can

comment:4 Changed 5 months ago by dgoulet

Reviewer: mikeperry

comment:5 Changed 5 months ago by mikeperry

Status: needs_reviewneeds_information

This looks simple enough to me. One question though: why not roll time forward enough for scale_active_circuits() to get called, so that we at least verify that it does not interfere with cell delivery on the channel? Unless that doesn't make sense, and there is no possible way it could.

Otherwise, merge ready.

comment:6 Changed 5 months ago by nickm

Resolution: fixed
Status: needs_informationclosed

Per discussion on IRC, I'd rather not actually trigger ewma scaling code here, since it was only ever triggered by accident, and this test doesn't actually do anything to check it for correctness.

Otherwise, merge ready.

Merging this to master.

Note: See TracTickets for help on using tickets.