Reduce the QoS to 0 when sending retained messages. #53

mcollina · 2016-06-08T09:10:28Z

Fixes #52

coveralls · 2016-06-08T09:11:56Z

Coverage increased (+0.3%) to 96.984% when pulling 088bec3 on qos-retain into 5b39bf3 on master.

bblumberg · 2016-06-08T14:32:23Z

Hi Matteo, thanks for the quick response. I got different behavior, but not the expected behavior.

When using redis or level based persistence, the 'device/tester/variable/win' topic was not published on reconnection. NOTE: if I removed the LWT from the connection, then it did work as expected.
when using memory based persistence, I got the following error:

client error tester no such packet Error: no such packet
at MemoryPersistence.outgoingUpdate (/data/users/bblumberg/Projects/aedes-broker/node_modules/aedes-persistence/persistence.js:187:6)
at MQEmitter.deliverQoS (/data/users/bblumberg/Projects/aedes-broker/node_modules/aedes/lib/client.js:103:28)
at goNoResultsArray (/data/users/bblumberg/Projects/aedes-broker/node_modules/fastparallel/parallel.js:111:18)
at MQEmitter.parallel as _parallel
at MQEmitter._do (/data/users/bblumberg/Projects/aedes-broker/node_modules/mqemitter/mqemitter.js:127:8)
at MQEmitter.emit (/data/users/bblumberg/Projects/aedes-broker/node_modules/mqemitter/mqemitter.js:110:10)
at PublishState.emitPacket (/data/users/bblumberg/Projects/aedes-broker/node_modules/aedes/aedes.js:147:18)
at makeCall (/data/users/bblumberg/Projects/aedes-broker/node_modules/fastseries/series.js:113:10)
at PublishState.ResultsHolder.release (/data/users/bblumberg/Projects/aedes-broker/node_modules/fastseries/series.js:96:9)
at ResultsHolder.release (/data/users/bblumberg/Projects/aedes-broker/node_modules/fastparallel/parallel.js:183:12)
at Aedes.parallel as _parallel
at doneEnqueue (/data/users/bblumberg/Projects/aedes-broker/node_modules/aedes/aedes.js:192:14)
at MemoryPersistence.subscriptionsByTopic (/data/users/bblumberg/Projects/aedes-broker/node_modules/aedes-persistence/persistence.js:138:3)
at PublishState.enqueueOffline (/data/users/bblumberg/Projects/aedes-broker/node_modules/aedes/aedes.js:159:27)
at makeCall (/data/users/bblumberg/Projects/aedes-broker/node_modules/fastseries/series.js:113:10)
at ResultsHolder.release (/data/users/bblumberg/Projects/aedes-broker/node_modules/fastseries/series.js:96:9)
25

So, sorry that I can't verify the fix using my test case. Thanks for your persistence on this problem

bb

mcollina · 2016-06-08T21:44:53Z

This should be it, check again.

much more complex that I thought, with two bugs one on top of the other.

coveralls · 2016-06-08T21:46:18Z

Coverage increased (+0.3%) to 96.984% when pulling 5c65006 on qos-retain into 5b39bf3 on master.

bblumberg · 2016-06-09T01:45:00Z

Thanks for your work on this. It is very much appreciated! Progress: no
crash on memory based persistence. I am still not getting the retained
messages on reconnect however. Here is a simple run using mosquitto,
showing how my example does get the retained messages when it starts up the
second time.

bb

Bruces-MacBook-Air:ab bruce$ node mqtt-test3.js

connecting tester

connected: undefined {

"cmd": "connack",

"retain": false,

"qos": 0,

"dup": false,

"length": 2,

"topic": null,

"payload": null,

"sessionPresent": false,

"returnCode": 0

}

message on topic device/tester/status online

message on topic device/tester/variable/win
{"item":{"type":"int","value":73}}

^C

Bruces-MacBook-Air:ab bruce$ node mqtt-test3.js

connecting tester

connected: undefined {

"cmd": "connack",

"retain": false,

"qos": 0,

"dup": false,

"length": 2,

"topic": null,

"payload": null,

"sessionPresent": false,

"returnCode": 0

}

message on topic device/tester/status offline

message on topic device/tester/status online

message on topic device/tester/variable/win
{"item":{"type":"int","value":73}}

message on topic device/tester/variable/win
{"item":{"type":"int","value":54}}

On Wed, Jun 8, 2016 at 5:44 PM, Matteo Collina notifications@github.com
wrote:

This should be it, check again.

much more complex that I thought, with two bugs one on top of the other.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#53 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGz5auIFvGCb_UBnMqYNohnKYK-_6ddqks5qJzfWgaJpZM4Iwv4_
.

. *.* .

Bruce Blumberg *| Principal Software Engineer
*Rethink Robotics, Inc *
main: 617.500.2487 | cell: 978.430.2206

mcollina · 2016-06-09T09:31:28Z

@bblumberg should be good now, let me know.

coveralls · 2016-06-09T09:31:53Z

Coverage increased (+0.3%) to 96.998% when pulling 44abcf0 on qos-retain into 5b39bf3 on master.

bblumberg · 2016-06-09T14:50:45Z

Cool. Here is what I am observing:

with memory based persistence: things work as expected. The client gets
1 copy of retained messages when it connects
with redis based persistence: things work as expected. The client gets 1
copy of the retained messages when it connects.
with level based persistence: the client gets 2 copies of retained
messages on connection. Since it is a qos:1 message getting multiple copies
is ok, although surprising to see different behavior.

Overall, one thing I did notice was that timing issues do complicate
things. In the test case, I sent you, often the retained copy of the
message would be received after the newly published message. This
disappeared once I put a bit of a delay before publishing on the topic.

So I would verify that your fixes addressed the problem I was seeing with
retained messages. Its your call whether (3) is ok or should be looked into.

Thanks again for your quick work on this and being patient with my test
cases.

bb

On Thu, Jun 9, 2016 at 5:31 AM, Matteo Collina notifications@github.com
wrote:

@bblumberg https://github.com/bblumberg should be good now, let me know.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#53 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGz5arYx2igABBkHSuNOTlFVvSNAyg_Mks5qJ91ygaJpZM4Iwv4_
.

. *.* .

Bruce Blumberg *| Principal Software Engineer
*Rethink Robotics, Inc *
main: 617.500.2487 | cell: 978.430.2206

mcollina · 2016-06-09T15:15:32Z

should be look into. Possibly a different issue.

Overall, one thing I did notice was that timing issues do complicate
things. In the test case, I sent you, often the retained copy of the
message would be received after the newly published message. This
disappeared once I put a bit of a delay before publishing on the topic.

In this PR the following are executed in order:

authorizing a subscription
storing the offline subscriptions on the persistence
added a listener to the backing broker
suback and start sending all the retain messsages

The retain messages are loaded asynchronously, so there might be some messages that slips in.

We need to start sending messages after SUBACK, and deferring any packets matching that subscription till after we have sent all the retained messages.

bblumberg · 2016-06-09T18:24:46Z

Makes sense.

bb

On Thu, Jun 9, 2016 at 11:15 AM, Matteo Collina notifications@github.com
wrote:

should be look into. Possibly a different issue.

Overall, one thing I did notice was that timing issues do complicate
things. In the test case, I sent you, often the retained copy of the
message would be received after the newly published message. This
disappeared once I put a bit of a delay before publishing on the topic.

In this PR the following are executed in order:

authorizing a subscription

storing the offline subscriptions on the persistence

added a listener to the backing broker

suback and start sending all the retain messsages

The retain messages are loaded asynchronously, so there might be some
messages that slips in.

We need to start sending messages after SUBACK, and deferring any packets
matching that subscription till after we have sent all the retained
messages.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#53 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AGz5aoZRWnpi2VpOV9piemwfSeD-hONHks5qKC4WgaJpZM4Iwv4_
.

. *.* .

Bruce Blumberg *| Principal Software Engineer
*Rethink Robotics, Inc *
main: 617.500.2487 | cell: 978.430.2206

Reduce the QoS to 0 when sending retained messages.

088bec3

Store subscriptions before adding the listener to MQEmitter

5c65006

Avoid wrong deduplication of retain messages

44abcf0

mcollina merged commit b77eb38 into master Jun 10, 2016

mcollina deleted the qos-retain branch June 10, 2016 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the QoS to 0 when sending retained messages. #53

Reduce the QoS to 0 when sending retained messages. #53

mcollina commented Jun 8, 2016

coveralls commented Jun 8, 2016 •

edited

Loading

bblumberg commented Jun 8, 2016

mcollina commented Jun 8, 2016

coveralls commented Jun 8, 2016 •

edited

Loading

bblumberg commented Jun 9, 2016

mcollina commented Jun 9, 2016

coveralls commented Jun 9, 2016 •

edited

Loading

bblumberg commented Jun 9, 2016

mcollina commented Jun 9, 2016

bblumberg commented Jun 9, 2016

Reduce the QoS to 0 when sending retained messages. #53

Reduce the QoS to 0 when sending retained messages. #53

Conversation

mcollina commented Jun 8, 2016

coveralls commented Jun 8, 2016 • edited Loading

bblumberg commented Jun 8, 2016

mcollina commented Jun 8, 2016

coveralls commented Jun 8, 2016 • edited Loading

bblumberg commented Jun 9, 2016

mcollina commented Jun 9, 2016

coveralls commented Jun 9, 2016 • edited Loading

bblumberg commented Jun 9, 2016

mcollina commented Jun 9, 2016

bblumberg commented Jun 9, 2016

coveralls commented Jun 8, 2016 •

edited

Loading

coveralls commented Jun 8, 2016 •

edited

Loading

coveralls commented Jun 9, 2016 •

edited

Loading