Delayed emit timing of `connect` event. #866

ogis-fujiwara · 2018-09-04T08:07:15Z

Emit connect event after processing of outgoingStore is completed.

Problem:
When client publish new message during proccessing of publish/pubrel messages in outgoingStore, the newly published message interrupts the process. As a result, messages are not sent by published time order.

Outcomes:
Above message order problem is fixed by this commit. Added unit test verifies this case.

Emit `connect` event after processing of `outgoingStore` is completed. Problem: When client publish new message during proccessing of publish/pubrel messages in `outgoingStore`, the newly published message interrupts the process. As a result, messages are not sent by published time order. Outcomes: Above message order problem is fixed by this commit. Added unit test verifies this case.

mcollina

Good spot, you'd also need to change

MQTT.js/lib/client.js

Line 202 in 5379f1c

this.on('connect', this._setupPingTimer)

. As we need this to be set up as soon as possible after connection, as sending all the messages in the store might take some time.

mcollina · 2018-09-04T08:11:04Z

lib/client.js

  // Mark connected on connect
-  this.on('connect', function () {
+  this._onConnect = function () {


can you move this to a top level function instead?

Yes, I will do that.

mcollina · 2018-09-04T08:11:15Z

lib/client.js

@@ -141,6 +144,7 @@ function MqttClient (streamBuilder, options) {
    this.once('close', remove)
    outStore.on('end', function () {
      that.removeListener('close', remove)
+      that.connectEmitter()


can you just emit('connect')?

connack packet is mandatory parameter when emitting connect.
So I introduced that.connectEmitter() to capture connack packet.
Instead of do that, I can store connack packet and pass it to emit connect.
Which one is better?

codecov · 2018-09-04T08:16:12Z

Codecov Report

Merging #866 into master will increase coverage by 0.03%.
The diff coverage is 93.61%.

@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
+ Coverage    94.1%   94.13%   +0.03%     
==========================================
  Files           8        8              
  Lines         763      767       +4     
  Branches      190      190              
==========================================
+ Hits          718      722       +4     
  Misses         45       45

Impacted Files	Coverage Δ
lib/client.js	`97.48% <93.61%> (+0.01%)`	⬆️

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 d138144...69d6356. Read the comment docs.

Called `this._setupPingTimer()` from `_onConnect()`. Modified `on('connect')` handler for resubscribe as top level function. The function is called from `_onConnect()`.

ogis-fujiwara · 2018-09-05T02:21:13Z

As we need this to be set up as soon as possible after connection, as sending all the messages in the store might take some time.
I will update the code as follows:

Before stored message processing, this._setupPingTimer() is called.
In addition, resubscribe is called. Because a client might have subscribed the same topic in the stored message.

ogis-fujiwara · 2018-09-05T04:04:16Z

I've updated all mentioned above.

Travis test was passed when I pushed changes to my topic branch. See below.
https://travis-ci.org/ogis-fujiwara/MQTT.js/builds/424621763
Would you restart failed tests?

mcollina · 2018-09-05T11:00:17Z

lib/client.js

+    this.connectEmitter = function () {
+      that.emit('connect', packet)
+    }
+    this._onConnect()


You can pass the packet to _onConnect(packet) and wrap it in the closures there.

Thanks, I will commit it.

ogis-fujiwara · 2018-09-06T01:02:32Z

I've commited it.
All Travis test against my branch was passed as below.
https://travis-ci.org/ogis-fujiwara/MQTT.js/builds/425060666

mcollina

@mqttjs/core I think this is semver-major. What do you all think?

Are there any other semver-major changes that we might want to do? (apart from #827).

mcollina · 2018-09-06T08:02:12Z

lib/client.js

-    storeDeliver()
-  })
+  // True if connection is first time.
+  this.firstConnection = true


can you make this _firstConnection? this should ideally be private.

I committed this. Could you please review?

RangerMauve · 2018-09-06T14:32:08Z

I could see this changing assumptions made in existing codebases. +1 to it being semver-major.

mcollina

LGTM

ogis-fujiwara · 2018-09-10T01:38:17Z

Thank you very much for merging.

Delayed emit timing of `connect` event.

mcollina reviewed Sep 4, 2018

View reviewed changes

Modified _onConnect() as top level function.

e46fa21

Called `this._setupPingTimer()` from `_onConnect()`. Modified `on('connect')` handler for resubscribe as top level function. The function is called from `_onConnect()`.

mcollina reviewed Sep 5, 2018

View reviewed changes

Passed packet to _onConnect().

8eb6423

mcollina reviewed Sep 6, 2018

View reviewed changes

Change this.firstConnection to private member.

69d6356

mcollina approved these changes Sep 7, 2018

View reviewed changes

mcollina merged commit 6e88644 into mqttjs:master Sep 7, 2018

redboltz pushed a commit to redboltz/MQTT.js that referenced this pull request May 19, 2019

Merge pull request mqttjs#866 from ogis-fujiwara/msg-order-at-restore

55d5632

Delayed emit timing of `connect` event.

redboltz pushed a commit to redboltz/MQTT.js that referenced this pull request May 19, 2019

Merge pull request mqttjs#866 from ogis-fujiwara/msg-order-at-restore

5ab6175

Delayed emit timing of `connect` event.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delayed emit timing of `connect` event. #866

Delayed emit timing of `connect` event. #866

ogis-fujiwara commented Sep 4, 2018

mcollina left a comment

mcollina Sep 4, 2018

ogis-fujiwara Sep 5, 2018

mcollina Sep 4, 2018

ogis-fujiwara Sep 5, 2018 •

edited

codecov bot commented Sep 4, 2018 •

edited

ogis-fujiwara commented Sep 5, 2018

ogis-fujiwara commented Sep 5, 2018

mcollina Sep 5, 2018

ogis-fujiwara Sep 5, 2018

ogis-fujiwara commented Sep 6, 2018

mcollina left a comment

mcollina Sep 6, 2018

ogis-fujiwara Sep 7, 2018

RangerMauve commented Sep 6, 2018

mcollina left a comment

ogis-fujiwara commented Sep 10, 2018

Delayed emit timing of connect event. #866

Delayed emit timing of connect event. #866

Conversation

ogis-fujiwara commented Sep 4, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina Sep 4, 2018

Choose a reason for hiding this comment

ogis-fujiwara Sep 5, 2018

Choose a reason for hiding this comment

mcollina Sep 4, 2018

Choose a reason for hiding this comment

ogis-fujiwara Sep 5, 2018 • edited

Choose a reason for hiding this comment

codecov bot commented Sep 4, 2018 • edited

Codecov Report

ogis-fujiwara commented Sep 5, 2018

ogis-fujiwara commented Sep 5, 2018

mcollina Sep 5, 2018

Choose a reason for hiding this comment

ogis-fujiwara Sep 5, 2018

Choose a reason for hiding this comment

ogis-fujiwara commented Sep 6, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina Sep 6, 2018

Choose a reason for hiding this comment

ogis-fujiwara Sep 7, 2018

Choose a reason for hiding this comment

RangerMauve commented Sep 6, 2018

mcollina left a comment

Choose a reason for hiding this comment

ogis-fujiwara commented Sep 10, 2018

Delayed emit timing of `connect` event. #866

Delayed emit timing of `connect` event. #866

ogis-fujiwara Sep 5, 2018 •

edited

codecov bot commented Sep 4, 2018 •

edited