moscajs / mosca

MQTT broker as a module
mosca.io
3.2k stars 508 forks source link

Created clientError event (fixes #609) #615

Closed hicaro closed 7 years ago

hicaro commented 7 years ago

This change creates a new event called clientError which can be catch in the broker code by:

// fired when client encounters an error
server.on('clientError', function(error, client) {
    // handle the output message   
});

This way the user will not have unexpected messages coming from that.logger.warn(err);.

Thank you for the awesome library!

mcollina commented 7 years ago

Would you mind adding a unit test for this feature?

hicaro commented 7 years ago

@mcollina I need a little help.

I am having a hard time to reproduce the error in the tests. I will tell you why.

Basically, every time I had this error happening, it was as a result of the socket on the client side hanging up. It would cause a Error: read ECONNRESET on the client instance in the server.

However, I've tried it in many different ways to simulate this error. client.stream.end() and client.stream.destroy() were among them.

I even tried:

 var stream = net.createConnection(instance.opts.port);
 stream.once('connect', function(){
     stream.destroy();
  });

All of them seem to close the connection smoothly which does not cause any error.

On my machine, not using the testing script, if I create two different processes, one for the server and another one for the client, and then use kill -SIGINT <pid> I can create the error on the server.

What do you think I should do?

mcollina commented 7 years ago

you can either spawn a separate process using 'child_process' or just emit('error') on the client.

hicaro commented 7 years ago

@mcollina Could you rerun the test for Node.js:4 build? It failed not due to my code.

It timed out on mosca.persistence.LevelUp offline packets should delete the offline packets once streamed.