Impossible to drop connection with wrong connection parameters

[gfcittolin]

I've found an odd condition, where nodeS7 keeps trying to reconnect forever, and there's no way to stop it.

I've simulated the following with an S7-1200: If you create a connection with a wrong parameter (e.g. wrong slot), we'll open the TCP socket, onTCPConnect() ist called, but then the connection is rejected after the ISO-on-TCP connection because of the wrong parameter. error and close events are emitted, and they both correctly triggers the cleanup process by calling connectionReset() and resetNow(). The main problem happens with the connectTimeout from onTCPConnect(): There, the packetTimeout() gets called with he "connect" parameter, and this calls connectNow() again after some time. But as the parameters are wrong, the same error will happen again, and this will keep happening forever. There's no way to stop it but to kill the process.

I see two possible solutions here:

• Make dropConnection() to stop the loop: dropConnection() is the way we currently have to close the communication, so we could either:
  • set a flag on dropConnection() signalizing we're dead, so 'connectNow()' would just skip it, or
  • make dropConnection() clean the timer from packetTimeout(), preventing the connection reset to happen again
• Don't try to reconnect on these connection errors: This is actually the case when we have an invalid IP address, for example. On both cases (wrong IP and wrong slot), the connection callback is called, but on the first case it stops, and on the second one it keeps trying (even after the callback is called). So here, we could:
  • stop the connectTimeout timer from onTCPConnect() if we get an error event (on the connectError()).

The second solution seems more correct in my point of view, as it should stop doing anything if the connection callback is called with an error.

What do you think about it? I can create then a PR with the fix for this using the solution chosen.

P.S.: This seems to be the cause of the issues netsmarttech/node-red-contrib-s7#26 and netsmarttech/node-red-contrib-s7#29

[plcpeople]

My only concern with the second method is the situations where it might not auto-reconnect. For example, when connecting to an IBH or Hemholz netlink, a connection error connecting to a certain slot may mean the PLC at that MPI address (since the slot can map to MPI addresses) and we rely on nodeS7 to auto-reconnect in those situations without pushing that connection monitoring work to the application that uses it. Same with the IP address - it's hard to know whether the IP address is wrong or if the PLC just isn't reachable at the time, but we rely on it auto-connecting either way. How would your code work in these situations, and how would that work with your module?

[gfcittolin]

Same with the IP address - it's hard to know whether the IP address is wrong or if the PLC just isn't reachable at the time

As far as I can test here, currently it won't reconnect if the IP address is not reachable, and that's why I thought the second option would be more in sync to the current behavior. But maybe it's the case we have to change this too, then, and implement the first fix for it. Right now I handle this case on my implementation (by trying to reconnect when the callback returns with an error)

In any case, I can handle both, as far as the behavior is always consistent.

[gfcittolin]

After some loong time, I could finally come back to this. I've created a PR (#73) implementing the first solution, as it's safer in the way we have much less chance of breaking somebody's code by changing behavior. @plcpeople if you're ok with that, you can merge the PR and publish it :)

[gfcittolin]

@plcpeople could you publish it on npm, please? You're the one with powers for that :)