New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change heartbeat negotiation strategy #874

Merged
merged 1 commit into from Oct 2, 2017

Conversation

Projects
None yet
4 participants
@zjj

zjj commented Sep 6, 2017

if client_value > 0 then this value should be used as heartbeat.
because sometimes, the max value of client_value and the
server_value may too big for firmwall, the firmwall may have broken
off the connection.

Zhong Jiajia
update heartbeat determin policy when tune
if client_value > 0 then this value should be used as heartbeat.
because sometimes, the max value of client_value and the
server_value may too big for firmwall, the firmwall may have broken
off the connection.
@codecov

This comment has been minimized.

codecov bot commented Sep 6, 2017

Codecov Report

Merging #874 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #874   +/-   ##
=======================================
  Coverage   81.98%   81.98%           
=======================================
  Files          20       20           
  Lines        3664     3664           
  Branches      545      545           
=======================================
  Hits         3004     3004           
  Misses        513      513           
  Partials      147      147
Impacted Files Coverage Δ
pika/connection.py 88.37% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f643d5c...7c13a57. Read the comment docs.

@zjj zjj changed the title from update heartbeat determin policy when tune to update heartbeat determine policy when tune Sep 6, 2017

@lukebakken lukebakken added this to the 0.11.1 milestone Sep 6, 2017

@lukebakken lukebakken self-assigned this Sep 6, 2017

@michaelklishin

This comment has been minimized.

Contributor

michaelklishin commented Oct 2, 2017

This is generally in line with what Bunny and Java client do (https://github.com/ruby-amqp/bunny/blob/master/lib/bunny/session.rb#L1172, https://github.com/ruby-amqp/bunny/blob/master/lib/bunny/session.rb#L1259) but I'd like to take a closer look.
For example, when only one of the peers disables heartbeats, Bunny will use the non-zero value of the two.

@michaelklishin

This comment has been minimized.

Contributor

michaelklishin commented Oct 2, 2017

I confirmed that Java client does the same thing (which is not surprising given that with Bunny we tried to copy most of those decisions).

Therefore Pika should, too.

@michaelklishin michaelklishin changed the title from update heartbeat determine policy when tune to Change heartbeat negotiation strategy Oct 2, 2017

@zjj

This comment has been minimized.

zjj commented Oct 2, 2017

@michaelklishin leave some flexibility to users ...

@michaelklishin michaelklishin merged commit 7c13a57 into pika:master Oct 2, 2017

4 checks passed

codecov/patch 100% of diff hit (target 81.98%)
Details
codecov/project 81.98% (+0%) compared to f643d5c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

michaelklishin added a commit that referenced this pull request Oct 2, 2017

Make heartbeat (frame_max, channel_max) negotiation consistent with r…
…eference client

Java client and some others (Bunny, .NET) use a slightly different
negotiation logic. See #874 comments for details.

With this change, in some cases when previously heartbeats were disabled,
they will be enabled now. Arguably this is an improvement because disabling
heartbeats is rarely a good idea (unless all machines run with sensible
TCP keepalive settings, which is a rare thing to come by).

Closes #874.
@michaelklishin

This comment has been minimized.

Contributor

michaelklishin commented Oct 2, 2017

@zjj I don't think "flexibility" is a good enough justification for inconsistent behaviour between clients when it comes to heartbeat negotiation. Or perhaps I misunderstood your comment.

I pushed an update that changes heartbeat negotiation logic to be the same as in Java client, which accomplishes the same goal: the smallest value of the two will be used when both client and server provide non-zero values.

@michaelklishin

This comment has been minimized.

Contributor

michaelklishin commented Oct 2, 2017

Releasing this in 0.11.1 seems OK: for the majority of users nothing will change. In other cases a lower value will be used, which is safe unless the values are really low (< 5 seconds is known to produce false positives often enough). Arguably this is a bug fix: the logic is now consistent with that of multiple other widely used clients (Java, .NET, Bunny) and it's closer to "doing the right thing" if you consider what the goal of having heartbeats in the protocol is.

@zjj

This comment has been minimized.

zjj commented Oct 2, 2017

@michaelklishin your patch is ok, but I think it's not good for pika to have its own cleaver policy to choose a value. it's a user's duty ???

@jordanburke

This comment has been minimized.

jordanburke commented Dec 13, 2017

Is 0 supposed to still turn off heartbeats? If so it looks like there's still an issue related to recent changes in the 11.1 or 11.2 release:

    @staticmethod
    def _negotiate_integer_value(client_value, server_value):
        """Negotiates two values. If either of them is 0 or None,
        returns the other one. If both are positive integers, returns the
        smallest one.

        :param int client_value: The client value
        :param int server_value: The server value
        :rtype: int

        """
        if client_value == None:
            client_value = 0
        if server_value == None:
            server_value = 0

        # this is consistent with how Java client and Bunny
        # perform negotiation, see pika/pika#874
        if client_value == 0 or server_value == 0:
            val = max(client_value, server_value)
        else:
            val = min(client_value, server_value)

        return val

With this there's no way 0 can turn off heartbeats, unless the server is set to 0 as well which invalidates the purpose of the 0 switch. Else if the functionality has changed the doc should be updated to indicate that 0 no longer switches off heartbeats.

@zjj

This comment has been minimized.

zjj commented Dec 20, 2017

@jordanburke see Michael's commit 3027890

gbartl pushed a commit to gbartl/pika that referenced this pull request Jan 27, 2018

Make heartbeat (frame_max, channel_max) negotiation consistent with r…
…eference client

Java client and some others (Bunny, .NET) use a slightly different
negotiation logic. See pika#874 comments for details.

With this change, in some cases when previously heartbeats were disabled,
they will be enabled now. Arguably this is an improvement because disabling
heartbeats is rarely a good idea (unless all machines run with sensible
TCP keepalive settings, which is a rare thing to come by).

Closes pika#874.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment