Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broker Id should be updated when streamed #98

Merged
merged 1 commit into from
Apr 2, 2017
Merged

Conversation

GavinDmello
Copy link
Collaborator

Related to #96
@mcollina @nguyenthenguyen Please verify

@coveralls
Copy link

coveralls commented Apr 1, 2017

Coverage Status

Coverage increased (+0.004%) to 97.42% when pulling 3d49536 on fixBrokerId into 3f8d6a7 on master.

@mcollina mcollina merged commit d151fbf into master Apr 2, 2017
@nguyenthenguyen
Copy link
Contributor

I have tested but not fix will problems. I have 1 aedes instance:
Step 1: aedes start with id1: HJmwY81Te
Step 2: All client connect to server id1 HJmwY81Te
Step 3: Client subcribe will topic testdevice/status to server id1 HJmwY81Te
Step 4: aedes restart with id2: XmmkY21xj for some reason, ex: server crash ...
Step 5: All client reconnect to server id2 XmmkY21xj
Step 6: Client subcribe will topic testdevice/status to server id2 XmmkY21xj
Step 7: server id2 XmmkY21xj start clean will https://github.com/mcollina/aedes/blob/master/aedes.js#L80
Step 8: All will topic get from persistence with old id1 HJmwY81Te process by https://github.com/mcollina/aedes/blob/master/aedes.js#L92
Step 9: needsPublishing function set to true by https://github.com/mcollina/aedes/blob/master/aedes.js#L94
Step 10: server id2 XmmkY21xj publish will https://github.com/mcollina/aedes/blob/master/aedes.js#L104
Step 11: server id2 not delete old will by https://github.com/mcollina/aedes/blob/master/aedes.js#L109
because that.persistence.delWill will use id2 not id1
-> Bug: When server with id2 start _clearWillInterval will publish all will topic of id1 but all client is online not offline

@mcollina
Copy link
Collaborator

mcollina commented Apr 3, 2017

can you send a PR with a broken unit test that reproduces this error? Looking at the code, it seems correct, as the following sentence is not correct:

because that.persistence.delWill will use id2 not id1

it uses the client id, not the broker id.

@nguyenthenguyen
Copy link
Contributor

@mcollina Thanks!
But now, I'm not find a solution for this
that.persistence.delWill use broker id and client id? Ex: redis persistence
https://github.com/mcollina/aedes-persistence-redis/blob/master/persistence.js#L510

@GavinDmello
Copy link
Collaborator Author

@nguyenthenguyen @mcollina The old will is not being deleted. That's one issue.
@nguyenthenguyen What did you mean by will publish all will topic of id1 but all client is online not offline. I didn't quite understand

@mcollina
Copy link
Collaborator

mcollina commented Apr 3, 2017

@nguyenthenguyen I have reverted this change, as it was probably not correct (my bad). Can you please upload a script to reproduce the issue you are facing using MQTT.js or just mqtt-connection?

@nguyenthenguyen
Copy link
Contributor

@mcollina @GavinDmello Thanks!
Step 1: Start server.js
Step 2: start client1.js
Step 3: start client2.js
Step 4: stop server.js
Step 5: start server.js
Step 6: See client2.js log
screen shot 2017-04-03 at 16 12 40

server.js

var redisPersistence = require('aedes-persistence-redis');
var aedes = require('aedes')({
    persistence: redisPersistence({
	    "port": 6379,          
		"host": "127.0.0.1",
		"family": 4,
		"password": "",
		"db": 1
    }),
    heartbeatInterval: 1000 // more fast to test
});

var mqttServer = require('net').createServer(aedes.handle);

var mqttPort = 1883
mqttServer.listen(mqttPort,"0.0.0.0", function () {
  console.log('mqtt server listening on port', mqttPort)
});

client1.js

var mqtt = require('mqtt')
var client  = mqtt.connect('mqtt://localhost:1883',{
    will:{
        topic:"client1/status",
        payload: "client1 offline",
        qos: 0,
        retain: true
    }
})

client.on('connect', function () {
    console.log("Client 1 connected")
    client.publish('client1/status', 'client1 online')
})

client.on('message', function (topic, message) {
  console.log(message.toString())
})

client2.js

var mqtt = require('mqtt')
var client  = mqtt.connect('mqtt://localhost:1883',{
    will:{
        topic:"client2/status",
        payload: "client2 offline",
        qos: 0,
        retain: true
    }
})

client.on('connect', function () {
    console.log("Client 2 connected")
    client.subscribe('client1/status')
})

client.on('message', function (topic, message) {
  console.log(message.toString())
})

@GavinDmello
Copy link
Collaborator Author

GavinDmello commented Apr 3, 2017

@nguyenthenguyen Wills aren't getting deleted because the brokerId is old, so you're getting it on every heartbeat.
@mcollina Can we pass the brokerId to the delWill function ?

@mcollina
Copy link
Collaborator

mcollina commented Apr 3, 2017

@GavinDmello I think we might update the will (putWill) before sending it, so all of this would actually work.

@GavinDmello
Copy link
Collaborator Author

If brokerId is not the same, del the old will and put the new one and then publish ?

@mcollina
Copy link
Collaborator

mcollina commented Apr 3, 2017

we can just do putWill, as that should overwrite the previous value, then publish, then delete.
However, we might have to fix some of the persistences, i.e. pass in the brokerId as you said.

@GavinDmello
Copy link
Collaborator Author

@mcollina Yea. We'll have to pass those brokerIds to delWill.
@nguyenthenguyen Sorry about this. Didn't quite understand the issue at first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants