plcpeople / nodeS7

Node.JS library for communication to Siemens S7 PLCs
MIT License
356 stars 120 forks source link

Small detail (hole) in Auto Reconnection! #104

Closed flacohenao closed 4 years ago

flacohenao commented 4 years ago

Ok, this was funny to found!

Imagine that you have an user that submits the connection options to initiate a connection between the PLC and nodeS7.

Now, by default we have 5000ms on the reconnect option and wich is fine.. the connection started but the user realized that the connetion options were wrong.

nodeS7, seeing that the PLC is not reachable, starts the autoReconnection process with the wrong connection parameters.

Now, the user sees that he typed the wrong parameters and want to update them.. but don't forget that there is an underlying reconnection process being fired.

Normaly what the user woud do is call the "update()" function (his own API) that allow him to update the connection parameters, and that function calls internally the disconnect() function of nodeS7 that passes the parameter true on _disconnect(true) to indicate nodeS7 to finish the current connection and to not schedule a reconnection.

but it happens that the the user calls the disconnect() function right when the _reconnectTimer is waiting for being executed (5000ms) so... as the _connectionState === CONN_DISCONNECTED is true on that moment... disconnect() function did actually nothing.

What would be the right approach to handle that? a workarond would be possible looping inside update() function and watch for _connectionState !== 0 due to the isConnected() getter will be always false, and then call the disconnect() function.

meanwhile, I guess Would be possible to call the internal _destroy methods to do the job but will be risky to not fit any of the parameters.

what do you suggest?

flacohenao commented 4 years ago

Im very sorry...

I was testing on the 1.0.0-alpha.0 version...

I didn't see the clearTimeout(this._reconnectTimer) on the current version...

Closing issue

gfcittolin commented 4 years ago

That's the bad thing on working/testing without releases... it's hard to track where you are and where everybody else is :( What I usually do in these cases is to checkout the repo in a separate folder and npm link to where I'm working, so we can easily check-out new commits and make changes where needed

Hopefully we'll be able to release some solid beta releases in the near future to prevent this from happening!

Thank you btw for testing!