jisotalo / ads-client

Unofficial Node.js ADS library for connecting to Beckhoff TwinCAT automation systems using ADS protocol.
https://jisotalo.fi/ads-client/
MIT License
80 stars 17 forks source link

Async loop destroyer #78

Closed Hopperpop closed 2 years ago

Hopperpop commented 3 years ago

Regarding jisotalo/node-red-contrib-ads-client#12 This is not a full fix, but more a proposal to start working from. What I see as a problem is that the ads-client starts in some parts async retry loops (ex. reconnecting), but doesn't have a good way to destroy these functions. Calling "clearTimeout" is not enough because if this is called while the callback functions is still executing, it could still start a new timeout and the loop continues.

I added a mechanisme to destroy loops like this better for the reconnection and the _systemManagerStatePoller. There should be a function that fully destroys the ads-client with all the async loops, and I think that should be the disconnect function. The current problem is that the reconnection loop destroys itself because it calls the disconnect function (indirectly). Maybe a split up is needed between a private and public disconnect function.

As you understand the code better, I want to hear what your opinion is about this approach, and if this even make any sense.

jisotalo commented 3 years ago

Thanks @Hopperpop, I'll try to check it out some day with a thought

This surely needs some fixing. Luckily it hasn't really been a problem in other than Node-RED systems.

Hopperpop commented 3 years ago

I'm using it also without any issues, as the system is stable and don't need any restart/redeploy. It's only an issue on systems that destroys the client. In the past I was working on a test-rig that had a lot of these type of issues (memory leaks/ unclosed references/...), and it was a pain to find solutions or workarounds. That's why I push a little bit (and help) to get it solved.

jisotalo commented 3 years ago

I developed it a little bit further. However, haven't yet tested and there is still that problem with disconnect() call.

Probably we need to do, like you said, an internal _disconnect() function.

Do you have a good test setup for this when it's ready? We need to be sure everything works as they should when releasing this.

Hopperpop commented 3 years ago

At the moment the only thing I have are the node-red test flows I shared before. Nothing you can call a "proper" test. I have also been looking into async hooks to track the lifetime of async calls. But I don't have anything usefull to share at the moment. I good unit-test framework should be the best for these kind of issues, but the hardest part is the mocking of the server. If I find something I will share!

Hopperpop commented 3 years ago

I noticed that it's possible to test it with a simple script like this:

const ads = require('ads-client');

const client = new ads.Client({
    targetAmsNetId: '127.0.0.1.1.1', 
    targetAdsPort: 851,
    allowHalfOpen: true,
    disableBigInt: true
});

client.setDebugging(0);

client.connect()
.then(async res => {
    console.log(`Connected to the ${res.targetAmsNetId}`)

    function sleep(ms) {
    return new Promise(resolve => setTimeout(resolve, ms));
    }
    await sleep(1000);
    return await client.disconnect()
})
.then(() => {
    console.log('Disconnected')
})
.catch(err => {
    console.log('Something failed:', err)
})

With our new code it always exits the program directly. With the old code it sometime hangs with following messages:

ads-client: WARNING: Target is connected but not in RUN mode (mode: Config) - connecting to runtime (ADS port 851) failed
Connected to the 127.0.0.1.1.1
Disconnected
ads-client: WARNING: Connection was lost. Trying to reconnect...
ads-client: WARNING: Target is connected but not in RUN mode (mode: Config) - connecting to runtime (ADS port 851) failed
ads-client: PLC runtime reconnected successfully and all subscriptions were restored!
jisotalo commented 3 years ago

@Hopperpop I developed this further. What do you think? I created new private _connect(), _reconnect() and _disconnect() functions to handle reconnecting better.

See commit: https://github.com/jisotalo/ads-client/pull/78/commits/bac4728ce06698cb42d63fb0ac244ae58b6f6ca4 (the next one has updated docs + readme).

Needs testing before releasing.

jisotalo commented 2 years ago

Added minor bug fixes that I found when further developing ads-server (adding this new system there too).

jisotalo commented 2 years ago

Merged! Everything worked fine in my tests at least (local system and also with raspberry pi + node-red).

Hopperpop commented 2 years ago

Sorry for the slow reply. All the previous tests seems to work fine for me with the new code. I currently can't find any issue. Thanks!

jisotalo commented 2 years ago

Thanks! Let me know if you find any problems :)