Opened 3 years ago

Closed 18 months ago

#19476 closed defect (implemented)

Use conditional intervals in heartbeat_callback

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, heartbeat, usability, review-group-20
Cc: Actual Points:
Parent ID: Points:
Reviewer: isis Sponsor:


In ticket:19454#comment:4 i suggested some rationale about the (now removed) comment in the heartbeat callback function.

The attached patch uses the return value of log_heartbeat to choose between calling the callback after HeartbeatPeriod number of seconds or on the next second.

Child Tickets

Attachments (1)

0001-Use-the-return-value-for-choosing-intervals.patch (1.0 KB) - added by cypherpunks 3 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by cypherpunks

Status: newneeds_review

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???

comment:3 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:4 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:5 Changed 20 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:6 Changed 19 months ago by nickm

Keywords: tor-relay heartbeat usability added
Milestone: Tor: unspecifiedTor: 0.3.2.x-final

comment:7 Changed 19 months ago by nickm

Keywords: review-group-20 added

Creating review-group-20

comment:8 Changed 19 months ago by nickm

Is this a functional change? What's the impact here?

comment:9 Changed 19 months ago by cypherpunks

The current behavior is to always call the heartbeat callback every HeartbeatPeriod seconds even if logging of the heartbeat has failed.

The patch ensures the heartbeat callback is only called every HeartbeatPeriod seconds if logging of the heartbeat message was successful. Otherwise it will report to the scheduler that the callback should be called again in the next second.

The only case where log_heartbeat returns != 0 is

103   if (public_server_mode(options) && !hibernating) {
104     /* Let's check if we are in the current cached consensus. */
105     if (!(me = router_get_my_routerinfo()))
106       return -1; /* Something stinks, we won't even attempt this. */
107     else
108       if (!node_get_by_id(me->cache_info.identity_digest))
109         log_fn(LOG_NOTICE, LD_HEARTBEAT, "Heartbeat: It seems like we are not "
110                "in the cached consensus.");
111   }

So in the case when 'something stinks' the patch reschedules calling the callback in the next second instead of after HeartbeartPeriod seconds.

comment:10 Changed 18 months ago by isis

Hi! I applied cypherpunks' patch in my bug19476 branch. It mostly LGTM.

I made the following small changes in that branch:

  • Instead of rescheduling for the next second—if the writing the heartbeat log message failed, retry MIN_HEARTBEAT_PERIOD seconds later. (The documentation for MIN_HEARTBEAT_PERIOD states that it is the "Lowest allowable value for HeartbeatPeriod; if this is too low, we might expose more information than we're comfortable with.")
  • Documented this behaviour in the heartbeat_callback() docstring.
  • Added a changes file.
Last edited 18 months ago by isis (previous) (diff)

comment:11 Changed 18 months ago by isis

Reviewer: isis

I set myself as the reviewer for cypherpunks' patch, but someone else should review my changes!

comment:12 Changed 18 months ago by teor

Status: needs_reviewmerge_ready

Looks good to me. Every 30 minutes is plenty.

comment:13 Changed 18 months ago by cypherpunks

LGTM2. Using MIN_HEARTBEAT_PERIOD is indeed better.

comment:14 Changed 18 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master; thanks, everybody

Note: See TracTickets for help on using tickets.