Cloud-Automation / node-modbus

Modbus TCP Client/Server implementation for Node.JS
471 stars 175 forks source link

Error On Sucessive WriteSingleRegister #100

Closed lfernando-silva closed 7 years ago

lfernando-silva commented 7 years ago

Hello. I made an interface to user push a button (a form) and it sends a WriteSingleRegister (to one station) command and it executes the follow command:

function writeSingleRegister(client) {
    var e = parseInt(client.address); //like 13
    var c = parseInt(client.content); // like 42
    return client.writeSingleRegister(e, c)
        .then(function (resp) {
            return resp;
        }).then(function (resp) {
            return resp;
        }).catch(function (err) {
            console.log('Error write single registers');
            throw err;
        }).finally(function () {
            client.close();
            return null;
        });
}

The problem is, initally I didn't handle the possibility of the user press this button multiple times in a short interval, for example, 10 times in, 30s. After some executions, it throws the err in the promise catch, and it blocks the application. I received a Unhandled rejection (<{"err":"timeout"}>, no stack trace) and stop sending commands for a while. If I set Promise.timeout time, I get:

Unhandled rejection TimeoutError: operation timed out
    at module.exports.afterTimeout (C:\Visual Studio 2015\Projects\EnergiasWritePromise\EnergiasWritePromise\node_modules\bluebird\js\release\timers.js:46:19)
    at Timeout.timeoutTimeout (C:\Visual Studio 2015\Projects\EnergiasWritePromise\EnergiasWritePromise\node_modules\bluebird\js\release\timers.js:76:13)
    at tryOnTimeout (timers.js:228:11)
    at Timer.listOnTimeout (timers.js:202:5)

How can I solve this? Any tips are welcome!

stefanpoeter commented 7 years ago

As the error said the operation timed out. That could possible be because of a connection error. Listen on the callback on the connect method and on the error event. Is your client really online?

To avoid multiple requests beeing send if you really just want to handle one (like in a trigger) make it so:

var locked = false;
var handler = function (params) {

    if (locked) {
       return
    }

    locked = true;
    return client.writeSingleRegister(...).then(...).finally(function () {
        locked = false;
    })

}
lfernando-silva commented 7 years ago

In the case, I'm trying it with my own IP in the net (not localhost but 192.168 ... ), reading this modbus simulator registers.

For example, I tried block it for 5 seconds with

...
return Promise
            .resolve(client)
            .then(function (client) {
                setTimeout(function () {
                    return writeSingleRegister(client);
                }, 5000);
            })
...

between requests and the problem remained. Your sugestion did'n work as expected, but waiting for a time (near of 30s), the behaviour back to normal. It looks like as if had cached requests in somewhere (that aren't the event listeners as explained in issue 97,).

Maybe the solution is apply some block in frontend, but how much time? If stay cached in somewhere, I think the way is to clean this cache or something like that

stefanpoeter commented 7 years ago

Did you notice that you are closing the client connection after the request?

.finally(function () {
    client.close();
    return null;
});

This way the client could try to reconnect, that could cause some problems if you do it all the time.

lfernando-silva commented 7 years ago

Oh it's true. But this happens even when reconnect param is setted to false? Anyway, I taked it out, you're right, it's really not convenient.

But the behavior didn't change, as if it has something caching and making some delay after the 10th request. I don't think it's a problem at the library itself, but maybe in some call to nodejs core (like the net class). This 'magic' count (10) is really curious for me.

UPDATE:

I saw in debugger (at Visual Studio) that the socket.on('data')'s callback is called every time, even when this problems ocurrs. So, the data is not sended and it's cached, but this socket.write

var onSend = function (pdu) {
      this.log.debug('Sending pdu to the socket.')

      reqId += 1

      var head = Buffer.allocUnsafe(7)

      head.writeUInt16BE(reqId, 0)
      head.writeUInt16BE(this.protocolVersion, 2)
      head.writeUInt16BE(pdu.length + 1, 4)
      head.writeUInt8(this.unitId, 6)

      var pkt = Buffer.concat([head, pdu])

      currentRequestId = reqId

        socket.write(pkt)
    }.bind(this)

is called in the right moment. It reinforces the ideia that the problem is calling the native nodejs core, It may be a matter of configuration or parameter to pass to.

stefanpoeter commented 7 years ago

Is this really the code you are using?

function writeSingleRegister(client) {
    var e = parseInt(client.address); //like 13
    var c = parseInt(client.content); // like 42
    return client.writeSingleRegister(e, c)
        .then(function (resp) {
            return resp;
        }).then(function (resp) {
            return resp;
        }).catch(function (err) {
            console.log('Error write single registers');
            throw err;
        }).finally(function () {
            client.close();
            return null;
        });
}

If so, then get rid of the second then it doesn't make sense. Throwing an error in the catch function also makes no sense and closing the client after every request as stated earlier is useless. Why don't you just do it this way and see what is in the logs.:

function writeSingleRegister(client) {
    var e = parseInt(client.address); //like 13
    var c = parseInt(client.content); // like 42
    return client.writeSingleRegister(e, c)
        .then(function (resp) {
            console.log("result", resp)
         }).catch(function (err) {
            console.log("error", err);
        })
}

If you provide me more code, I probably can tell you whats wrong. What are your settings in the modbus simulator?

lfernando-silva commented 7 years ago

That's right, I called .then twice, it's just wrong type. When you talked about the software configurations, I realize the number of connections allowed by default on it is... ten! I setted it to 1000 and the problem is over, but this leads me to another problem: I'm creating a connection every single request... how to get the connection instance?

//ModbusClientHandler.js
//params comes from a trigger
var ModbusClientHandler = {
    executeWriteSingle: function (params) {
        var client = getClient(params); //getClient returns modbus.client.tcp.complete instance with content and mobus address
        client.connect();
        client.on('connect', function () {
            console.log(WriteSingleRegister(client));
            return null;
        });
        client.on('error', function (err) {
            console.log(err);

        });
    }
};

function writeSingleRegister(client) {
    var e = parseInt(client.address); //like 13
    var c = parseInt(client.content); // like 42
    return client.writeSingleRegister(e, c)
        .then(function (resp) {
            return resp;
        }).catch(function (err) {
            console.log('Error write single registers');
            throw err;
        }).finally(function () {
            client.close();
            return null;
        });
}
stefanpoeter commented 7 years ago

I am closing this since the original issue is resolved.

I'm creating a connection every single request... how to get the connection instance?

Just don't. Create a client connection and store it somewhere you can access it from your function.

lfernando-silva commented 7 years ago

That's okay, at least a way to follow working on. Thanks.