pubnub / dart

PubNub Dart SDK
Other
27 stars 15 forks source link

Presence - Heartbeat Interval Should Be Heartbeat / 2 - 1 #33

Open huonderv opened 3 years ago

huonderv commented 3 years ago

According to the PubNub REST API | Long running heartbeat calls, the heartbeatInterval should "automatically" be set to heartbeat / 2 -1:

if the client is set for a HB of x, the HBI is automatically set to x / 2 - 1. This enables, by default, that under excellent to average-bad network scenarios, the client should be able to "ping" the Presence server between 1 and 2 times before its HB timeout limit.

I believe that this is currently not reflected in the dart SDK.

Subscribe

Currently, the heartbeatInterval can be set via the SubscribeKeysetExtension:

https://github.com/pubnub/dart/blob/72f011d15e58d065824ce966266d022e164fb59b/pubnub/lib/src/subscribe/extensions/keyset.dart#L15

This heartbeatInterval is then directly used as heartbeat value when subscribing to a channel in SubscribeParams:

https://github.com/pubnub/dart/blob/72f011d15e58d065824ce966266d022e164fb59b/pubnub/lib/src/subscribe/_endpoints/subscribe.dart#L46

*Shouldn't the heartbeat that is sent in this request be `(heartbeatInterval + 1) 2`?**

PresenceWidget

In the PresenceWidget, the same heartbeatInterval is used both for setting the periodic Timer

   timer = Timer.periodic(
        Duration(seconds: widget.heartbeatInterval), _sendHeartbeat);

and as hearbeat sent to the server

  Future<void> _sendHeartbeat(Timer timer) async {
    ...
    await _pubnub.announceHeartbeat(
      heartbeat: widget.heartbeatInterval, // <== here
      channels: channels,
      channelGroups: channelGroups,
      keyset: widget.keyset,
      using: widget.using,
    );
  }

*Shouldn't also there the heartbeat rather be `(heartbeatInterval + 1) 2`?**

Because with the current implementation, we got a lot of timeout presence events when the internet connection was not perfect.

Suggestion

I would strongly suggest to introduce an additional presenceTimeout and automatically set it to (heartbeatInterval + 1) * 2. Or the other way around and set the heartbeatInterval based on presenceTimeout, as e.g. done in the JavaScript SDK:

https://github.com/pubnub/javascript/blob/499b14bf6e0802c6389e6038add9bfd37af45601/src/core/components/config.js#L263

  setPresenceTimeout(val: number): this {
    ...
    this.setHeartbeatInterval(this._presenceTimeout / 2 - 1);
    return this;
  }

https://github.com/pubnub/javascript/blob/403e14cac7da6820239749686251080be4760022/src/core/endpoints/subscribe.js#L56

  const params: Object = {
    heartbeat: config.getPresenceTimeout(),
  };
are commented 3 years ago

Hi there! Thanks for reporting this issue. You are absolutely right! We will include this fix in the next release.