Connection heartbeat arg description doesn't match implementation

[vitaly-krugl]

The heartbeat arg description https://github.com/agoragames/haigha/commit/73cd9b1736fb72f3a140b6317b526a2111e38a7f#diff-a440ae8b97ad1a5653b91743003e498aR334 is

heartbeat Default None (disabled). If 0, broker assigned. If >0, negotiated with broker.

The implementation appears to be that if user passed heartbeat=None, then the heartbeat value is broker-assigned; if user passed heartbeat=0, then heartbeats would be disabled. The quoted description is the reverse of what actually happens in the code.

        if self.connection._heartbeat is None:
            self.connection._heartbeat = method_frame.args.read_short()

[vitaly-krugl]

CC @josegonzalez

[josegonzalez]

I don't know. It seems like the documentation is right, but its hard to figure it out based on the naming of the heartbeat variable across the codebase.

[vitaly-krugl]

The negotiation logic is:

        if self.connection._heartbeat is None:
            self.connection._heartbeat = method_frame.args.read_short()

which accepts broker's value if user's value is None. However, the description says "Default None (disabled). If 0, broker assigned.", which is absolutely not the same thing.

[josegonzalez]

The logic here actually doesn't send the heartbeat either way though?

        if self.connection._heartbeat:
            if time.time() >= (self._last_heartbeat_send + 0.9 *
                               self.connection._heartbeat):
                self.send_frame(HeartbeatFrame(self.channel_id))
                self._last_heartbeat_send = time.time()

[vitaly-krugl]

Correct, but the broker cannot pass the value None in Connection.Tune; the value None could only have come from the user, and after negotiation self.connection._heartbeat will not be None. If the user passes None, the only way that self.connection._heartbeat could end up as 0 after negotiation is if the broker passed 0 in Connection.Tune

[josegonzalez]

True, but isn't _heartbeat set by the consumer?

[vitaly-krugl]

It's set from user's arg here: https://github.com/agoragames/haigha/blob/a3ce85536f3de902ef1056304237e873cde6a8fe/haigha/connection.py#L67 and then overridden here https://github.com/agoragames/haigha/blob/a3ce85536f3de902ef1056304237e873cde6a8fe/haigha/connection.py#L608

[josegonzalez]

@vitaly-krugl note: since we deployed this change in production, we no longer get heartbeat timeouts in related services. This definitely merits further research though.

[vitaly-krugl]

@josegonzalez, this makes sense, because your change heartbeat=0 disables heartbeats on both client and broker sides of the connection.

[josegonzalez]

So are the docs wrong?

[vitaly-krugl]

@josegonzalez, yes the docs are wrong, and that's the gist of this issue.

[josegonzalez]

@vitaly-krugl so one reason we made this change was an upgrade to RabbitMQ moved the timeout from 5 minutes to 1 minute, impacting some of our longer running jobs (map and ticket processing). What you're saying makes it sound like there is an issue in our networking in regards to sending the heartbeats maybe, especially if the default of None should make it respect server heartbeats...

[josegonzalez]

Hmmm, seems like us not sending heartbeats was unrelated. So then yes, the docs here are almost certainly wrong, heh.