Fixed resource management on manual reconnect. #734

redboltz · 2017-12-06T04:09:24Z

Store._inflights set to {} instead of null to prepare manual reconnect.
Deferred the timing of manual reconnect calling if reconnect() called
during disconnecting process.

NOTE: reconnect() function usually called from 'close' or 'error' handler.

Store._inflights set to {} instead of null to prepare manual reconnect. Deferred the timing of manual reconnect calling if reconnect() called during disconnecting process. NOTE: reconnect() function usually called from 'close' or 'error' handler.

codecov · 2017-12-06T04:16:49Z

Codecov Report

Merging #734 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   92.88%   93.03%   +0.14%     
==========================================
  Files           8        8              
  Lines         703      718      +15     
  Branches      173      178       +5     
==========================================
+ Hits          653      668      +15     
  Misses         50       50

Impacted Files	Coverage Δ
lib/client.js	`96.41% <100%> (+0.11%)`	⬆️

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 13f0219...f3f781b. Read the comment docs.

Defer reconnecting only the cae if during disconnecting but not disconnected yet. Added test for NOT deferring reconnect().

mcollina · 2017-12-06T10:45:33Z

lib/client.js

@@ -586,6 +586,9 @@ MqttClient.prototype.end = function (force, cb) {
    that.incomingStore.close(function () {
      that.outgoingStore.close(cb)
    })
+    if (that.deferredReconnect) {
+      that.deferredReconnect()


as deferredReconnect is only used internally, can you please add a _ prefix?

OK, I will do it.

mcollina · 2017-12-06T10:48:16Z

lib/store.js

@@ -115,7 +115,7 @@ Store.prototype.get = function (packet, cb) {
 */
 Store.prototype.close = function (cb) {
  if (this.options.clean) {
-    this._inflights = null
+    this._inflights = {}


I don't think this is generically correct. I think _we should not close store.close() in this scenario.

I agree with you. Store.close() is not an appropriate point to prepare reconnect().

When I revert this._inflights = null, the tests I added don't pass.
https://github.com/mqttjs/MQTT.js/pull/734/files#diff-16702ac352d8772fe21f4f9c1de7e900R2031

The connection is clean: true (default). In this case, I think that user should be able to call client.end() and then call client.reconnect() in the close handler.
However, call client.reconnect() or not is unpredictable, so I set _inflights to {} to prepare next client.reconnect() calling.

I think that store.close() should be called because client.end() is called because of #707 (comment)

Finally, I found the appropriate point to reset Store._inflights. It is reconnect() function. I renew the incomingStore and outgoingStore there. They could be user custom object so I call constructor explicitly.

Please check out my next commit.

Renew `Store` on reconnect instead of setting Store._inflights to `{}` on `Store.close()`.

redboltz · 2017-12-06T12:52:21Z

lib/client.js

-    that.deferredReconnect = null
+    that._deferredReconnect = null
+    that.incomingStore = new that.incomingStore.constructor(that.incomingStore.options)
+    that.outgoingStore = new that.outgoingStore.constructor(that.outgoingStore.options)


This constructor calling might be wrong. It assumes that incomingStore and outgoingStore have options, and their constructor has one parameter options.

I considered two more approaches.

Add a new interface like resetInglights to Store. And client calls it on reconnect().

Add reconnect() method restriction. It can be called only if client is created with the option clean: false. User knows clean setting. If user want to reconnect with clean: true, user can call new connect(). That means creating the new connection but it is ok for clean: true.

What do you think?

Approach 1 breaks existing user custom Store implementation. So Approach 2 is good. I will remove explicit constuctor calling code and add reconnect tests for both clean: true and clean: false.

I updated the code using Approach 2.

Added `reconnect()` restriction to documents. Updated `reconnect()` tests. They are for `clean: false`. Added `connect()` again tests. They are for `clean: true`.

mcollina · 2017-12-06T22:43:37Z

lib/client.js

@@ -633,16 +636,27 @@ MqttClient.prototype.removeOutgoingMessage = function (mid) {
 }

 /**
- * reconnect - connect again using the same options
+ * reconnect - connect again using the same options as connect()
+ * `incomingStore` and `outgoingStore` need to be ready before `reconnect()` calling.
 *
 * @returns {MqttClient} this - for chaining
 *
 * @api public
 */
 MqttClient.prototype.reconnect = function () {


We could add the two stores as an option to reconnect.

It's a good idea!
If stores are passed, reconnect() uses them, otherwise renew stores the same as connect().
If users want to reuse existing stores, hold the stores on the user side. And then, pass them to reconnect(). It's intuitive.

That includes `incomingStore` and `outgoingStore` similar to `connect()`. If stores passed, `reconnect()` uses them, otherwise renew default `Store`.

Updated `reconnect()` JavaScript comment.

redboltz · 2017-12-07T02:29:53Z

I forgot TypeScript update. I will do it soon.

redboltz · 2017-12-07T02:41:40Z

updated.

mcollina · 2017-12-09T11:38:05Z

Good work, landing!

redboltz · 2017-12-09T11:49:28Z

Thank you very much!!

Updated deferring condition.

43d3258

Defer reconnecting only the cae if during disconnecting but not disconnected yet. Added test for NOT deferring reconnect().

mcollina reviewed Dec 6, 2017

View reviewed changes

Added '_' prefix to deferredReconnect.

6b0a7f7

Renew `Store` on reconnect instead of setting Store._inflights to `{}` on `Store.close()`.

redboltz commented Dec 6, 2017

View reviewed changes

Removed constructor call.

e271142

Added `reconnect()` restriction to documents. Updated `reconnect()` tests. They are for `clean: false`. Added `connect()` again tests. They are for `clean: true`.

mcollina reviewed Dec 6, 2017

View reviewed changes

redboltz added 2 commits December 7, 2017 08:35

Added opts parameter to reconnect().

8adeaa7

That includes `incomingStore` and `outgoingStore` similar to `connect()`. If stores passed, `reconnect()` uses them, otherwise renew default `Store`.

Updated reconnect() typescript documents.

f3f781b

Updated `reconnect()` JavaScript comment.

mcollina merged commit 1eda784 into mqttjs:master Dec 9, 2017

mcollina mentioned this pull request Mar 21, 2018

Force callbacks when pingresp not received or when end called with true. #787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed resource management on manual reconnect. #734

Fixed resource management on manual reconnect. #734

redboltz commented Dec 6, 2017

codecov bot commented Dec 6, 2017 •

edited

Loading

mcollina Dec 6, 2017

redboltz Dec 6, 2017

mcollina Dec 6, 2017

redboltz Dec 6, 2017

redboltz Dec 6, 2017

redboltz Dec 6, 2017

redboltz Dec 6, 2017

mcollina Dec 6, 2017

redboltz Dec 6, 2017

redboltz Dec 6, 2017

redboltz commented Dec 7, 2017

redboltz commented Dec 7, 2017

mcollina commented Dec 9, 2017

redboltz commented Dec 9, 2017

Fixed resource management on manual reconnect. #734

Fixed resource management on manual reconnect. #734

Conversation

redboltz commented Dec 6, 2017

codecov bot commented Dec 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redboltz commented Dec 7, 2017

redboltz commented Dec 7, 2017

mcollina commented Dec 9, 2017

redboltz commented Dec 9, 2017

codecov bot commented Dec 6, 2017 •

edited

Loading