search

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
78 stars 18 forks source link

Let `unregisterAdsPort` resolve when `AMS_TCP_PORT_CLOSE` is received #158

Closed crishoj closed 1 day ago

crishoj commented 6 days ago

Possible fix for #157.

Also, this.socketWrite() is not asynchronous, so await doesn't have any effect.

crishoj commented 4 days ago

As @jisotalo rightly noted:

we just need to make sure it's ok to do this "fire and forget" port unregistering

crishoj commented 4 days ago

we just need to make sure it's ok to do this "fire and forget" port unregistering

Updated to resolve when AMS_TCP_PORT_CLOSE packet is received.

Works well locally.

@jisotalo, could you check if this approach seems sane? In particular, the timeout you suggested is not included.

crishoj commented 4 days ago

Also, in this PR, if the server doesn't end the TCP connection, it will stay open. So we would need to manually call socket.end() and/or socket.destroy() before resolving.

Testing the latest version of this PR locally, I was able to repeatedly connect and disconnect.

But I assume you're right — we should close the socket as well.

crishoj commented 4 days ago

Do you think we need a timeout as well?

jisotalo commented 4 days ago

That looks great, just like I was thinking I would create it! I will look into it more carefully later.

We haven't had timeout before, but now when thinking it out loud, maybe there should be one?

It's not very likely to happen, but it could be possible that the port unregister is never confirmed by remote, which could cause the disconnect to never resolve. Maybe if there is no response (or socket close) in settings.timeoutDelay, the client would call disconnectFromTarget(true) to force closing (and then resolve). And also clear the amsTcpCallback.

This of course increases complexity as we need to make sure to clear timers in all cases etc.

crishoj commented 4 days ago

Timeout added.

Ensuring that both the timer and the callback are cleared in all cases does increase complexity.

crishoj commented 4 days ago

Maybe if there is no response (or socket close) in settings.timeoutDelay, the client would call disconnectFromTarget(true) to force closing (and then resolve).

It seems disconnectFromTarget() already handles failures from unregisterAdsPort() by forcefully closing the connection.

jisotalo commented 3 days ago

Looking good! I'll check and merge as soon as I have time!

It seems disconnectFromTarget() already handles failures from unregisterAdsPort() by forcefully closing the connection.

You are right, it's already there!

jisotalo commented 1 day ago

Checked out and tests with tc2&tc3 run ok. And if in your tests the AdsRouterConsole works as well, I think wer are done here :)

crishoj commented 1 day ago

Thanks for checking!