Add support for outgoing heartbeats by jmglsn · Pull Request #72 · stomp-php/stomp-php

jmglsn · 2017-05-29T20:57:48Z

In #60 we see that heartbeats support might help to avoid broken connections or at least they can reduce the lack from error to notification.

This only includes support for outgoing beats, incoming beats are not yet included. I could imagine to use the same observer infrastructure to offer support for incoming beat detection.

- add observer based heartbeat emitter, which must be added to the connection if outgoing heartbeats are wanted - allow to use heartbeats when connection is reopened by server - fix older test cases to be more precise and fix preconditions

staabm · 2017-05-30T08:22:54Z

    }

+  /**
+   * Set the desired heartbeat for the connection.


we should spent a small sentence why one would want to use a heartbeat (which problem it solves conceptionally)

staabm · 2017-05-30T08:25:28Z

+    public function receivedFrame(Frame $frame);
+
+    /**
+     * Indicates that a frame has been transmitted.


the word transmit does not imply a direction, does it?

received fits as it describes a incoming frame, but transmitted isnt a good fit IMO.

maybe sentFrame or somthing?

staabm · 2017-05-30T08:25:59Z

+     * Adds new observers to the collection.
+     *
+     * @param ConnectionObserver $observer
+     * @return ConnectionObserverCollection


$this (same in other spots of this class)

I used to replace $this with the actual return type since some time now. I also see that at symfony code - I would just like to change it to something like @return ConnectionObserverCollection this collection - what do you think about that?

staabm · 2017-05-30T08:26:33Z

+     *
+     * @return ConnectionObserver[]
+     */
+    public function getObserver()


plural Observers

staabm · 2017-05-30T08:28:51Z

+     */
+    public function receivedFrame(Frame $frame)
+    {
+        if ($frame->getCommand() === 'CONNECTED') {


should we have constants for this magic strings?

staabm · 2017-05-30T08:29:22Z

+     */
+    private function getHeartbeats(Frame $frame)
+    {
+        $beats = $frame['heart-beat'];


do we need an isset ?

The ArrayAccess implementation of Frame will return null if the header is not available. That simplified some work with the headers.

staabm · 2017-05-30T08:33:33Z

        if (!@fwrite($this->connection, $data, strlen($data))) {
            throw new ConnectionException('Was not possible to write frame!', $this->activeHost);
        }
+        $this->getObserver()->transmittedFrame($stompFrame);


we shouldnt use the getter inside off this class... this is a very hot path of our lib .. just use $this->observer instead.

I have the same feeling for isConnected(), hasDataToRead() etc. within this class, but this shouldn't be changed with this PR. I guess we can speedup the client a lot when we prevent this calls and just inline the code (the methods should still be preserved for public consumption and BC)

That's right we should check that in our next release.

staabm · 2017-05-30T08:34:10Z

Great Job, just a few minor nits/questions!

jmglsn · 2017-05-31T17:24:37Z

@staabm - Thank you for the review 👍 I'll apply the notes within a few days.

jmglsn · 2017-05-31T21:04:03Z

Also updated the examples stomp-php/stomp-php-examples@fb02c79.

If everything is fine we can squeeze and merge this.

staabm · 2017-06-01T06:03:35Z

+ * file that was distributed with this source code.
+ */
+
+namespace Stomp\Network\Observer\Heartbeat;


This is a single class Namspace.. should we instead strip heartbeat from the NS and name the class HeartbeatEmitter ?

Yes, when I added it I thought about the Monitor but there will be never more than two, so it's fine to Prefix them.

staabm · 2017-06-01T07:15:23Z

+   * within an interval - to indicate that the connection is still stable. If client and server agree on a beat and
+   * the interval passes without any data activity / beats the connection will be considered as broken and closed.
+   *
+   * If you define a heartbeat, you must assure that your application will send data within the interval.


hmm isn't it exactly the opposite? in case you use heartbeats you dont need to send data in certain intervalls as the client will automatically sent a hearbeat to keep the connection alive..?

jein - without a heartbeat declaration you can send data whenever you want, you could even add some minutes of sleep between the communication.
With heartbeats enabled you must do something within the interval.
The emitter is not automatically attached to the connection. Maybe this description is not yet good enough yet. But it's not that easy, hm...

hm does it make sense to enable hearbeats but dont add the emitter?

I think so, for those who don't like to add the overhead and expect to send data within the interval.

staabm · 2017-06-01T07:17:36Z

+     *
+     * @return void
+     */
+    private function notifyBeat()


rememberBeat?

staabm · 2017-06-01T07:17:53Z

+    }
+
+    /**
+     * Returns the heart beat header.


hearbeat single word?

staabm · 2017-06-01T07:19:13Z

+        if ($frame->getCommand() === Emitter::FRAME_CLIENT_CONNECT) {
+            $beats = $this->getHeartbeats($frame);
+            $this->intervalClient = $beats[0];
+            $this->notifyBeat();


do we need enabled=true in this IF?

No, it should be possible to support the RabbitMQ "Implicit Connect" (https://www.rabbitmq.com/stomp.html) this works without a CONNECT frame - so the enabled flag is here to query the state, but not to control it. (see testEmitterActivatedIfServerRequestsBeatsAndNoConnectFrameWasSend)

staabm · 2017-06-01T07:20:17Z

+     *
+     * @param float $intervalUsage
+     */
+    public function setIntervalUsage($intervalUsage)


as this value should be set before the emitter gets active, should it be a constructor arg instead?

staabm · 2017-06-01T20:14:30Z

+    {
+        if ($this->isConnected()) {
+            return (!@fwrite($this->connection, "\n", 1) == true);
+        }


Intentionally not a strict comparison?

It's easy to see that this might return nothing good...
Good reason to wait until Tuesday before opening a PR the next time ;)

andrewbelcher and others added 2 commits May 25, 2017 22:03

Demonstrate failures with heartbeats.

2613f66

jmglsn added enhancement stomp-1.2 version 4.x labels May 29, 2017

jmglsn requested a review from staabm May 29, 2017 20:57

staabm reviewed May 30, 2017

View reviewed changes

improve code by applying review notes

ee5bf92

staabm reviewed Jun 1, 2017

View reviewed changes

improve code by applying review notes

1a4e51c

staabm reviewed Jun 1, 2017

View reviewed changes

fix invalid sendAlive return value

7c4e615

jmglsn merged commit c1118c1 into master Jun 11, 2017

jmglsn deleted the feature/heartbeats branch June 11, 2017 09:41

Uh oh!

Uh oh!

Conversation

jmglsn commented May 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm commented May 30, 2017

Uh oh!

jmglsn commented May 31, 2017

Uh oh!

jmglsn commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmglsn Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm May 30, 2017 •

edited

Loading

jmglsn Jun 1, 2017 •

edited

Loading