Opened 3 years ago

Closed 2 years 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:

Description

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 3 years ago by teor

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

Milestone renamed

comment:4 Changed 3 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 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:6 Changed 2 years ago by nickm

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

comment:7 Changed 2 years ago by nickm

Keywords: review-group-20 added

Creating review-group-20

comment:8 Changed 2 years ago by nickm

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

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

comment:11 Changed 2 years ago by isis

Reviewer: isis

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

comment:12 Changed 2 years ago by teor

Status: needs_reviewmerge_ready

Looks good to me. Every 30 minutes is plenty.

comment:13 Changed 2 years ago by cypherpunks

LGTM2. Using MIN_HEARTBEAT_PERIOD is indeed better.

comment:14 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master; thanks, everybody

Note: See TracTickets for help on using tickets.