Opened 20 months ago

Closed 7 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 20 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 20 months ago by cypherpunks

Status: newneeds_review

comment:2 Changed 20 months ago by nickm

Milestone: Tor: 0.2.???

comment:3 Changed 15 months ago by teor

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

Milestone renamed

comment:4 Changed 14 months 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 9 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:6 Changed 8 months ago by nickm

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

comment:7 Changed 8 months ago by nickm

Keywords: review-group-20 added

Creating review-group-20

comment:8 Changed 8 months ago by nickm

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

comment:9 Changed 8 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 7 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 7 months ago by isis (previous) (diff)

comment:11 Changed 7 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 7 months ago by teor

Status: needs_reviewmerge_ready

Looks good to me. Every 30 minutes is plenty.

comment:13 Changed 7 months ago by cypherpunks

LGTM2. Using MIN_HEARTBEAT_PERIOD is indeed better.

comment:14 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master; thanks, everybody

Note: See TracTickets for help on using tickets.