race condition on close method

[buraktt]

in lib/client.js line 602:

    that.server.emit("clientDisconnected", that, reason);

    callback();

Callback is called immediately after disconnect event is emitted. if a client disconnects dirty and connects before keep alive period ends, it ends up creating a race condition between clientDisconnected/clientConnected events if it's a busy environment.

Changing that line to:

    that.server.emit("clientDisconnected", that, function(){
      callback()
    });

and in lib/server.js rewriting clientDisconnected event as follows:

  that.on("clientDisconnected", function(client, callback) {

    if (!callback) {
      callback = nop;
    }

    if(that.modernOpts.publishClientDisconnect) {
      that.publish({
        topic: "$SYS/" + that.id + "/disconnect/clients",
        payload: client.id
      }, callback);
    } else {
      callback()
    }
    delete this.clients[client.id];
  });

This way it's guaranteed that callback is called after clientDisconnect is published.

[mcollina]

We cannot pass callbacks to events, as it might cause huge issues, as we have no guarantee that would be called.

Can you figure out an alternative solution?

[buraktt]

Setting client's reconnectPeriod longer than keep alive * 1.5 minimizes the occurrence of race condition but it doesn't eliminate it completely.

What do you mean by "we have no guarantee that would be called"? As far as I know assigning callbacks to events are valid and guaranteed to be executed. (Please correct me if I'm wrong)

[mcollina]

What do you mean by "we have no guarantee that would be called"? As far as I know assigning callbacks to events are valid and guaranteed to be executed. (Please correct me if I'm wrong)

@buraktt no. If anyone else attaches to 'clinedDisconnected', they can call the callback multiple times. So, we need to move that logic in server.js as an old school function instead of an event.

[buraktt]

@mcollina thanks for clarifying. I'll move it & submit a pr.