search

st-one-io / node-open-protocol

This node is an implementation of the Atlas Copco's Open Protocol. This node was created by Smart-Tech as part of the ST-One project.
GNU General Public License v3.0
39 stars 38 forks source link

Correct linklayer destroy call #8

Closed apcurtiss closed 5 years ago

apcurtiss commented 5 years ago

SessionControlClient close() method contained a call to "this.ll.destroy()", but LinkLayer only has a "_destroy()" method. Just added the missing underscore!

dforz commented 5 years ago

Bump, would like to get this landed so I don't have to fork!

gfcittolin commented 5 years ago

Hi guys, sorry for the delay. Actually, calling _destroy() is not recommended per specs of Stream itself. The correct way should be to implement/override it, and let nodejs call it when destroy() is called.

Problem is, this was added only on node 8.0, therefore I assume you are using an older version of node. Could you check if this is the case?

I don't know what would be the best way to handle both older and newer versions, need to check out on the web. Do you have any idea?

gfcittolin commented 5 years ago

I have read a bit about it, and it looks like the correct way is to call _destroy on older versions, and destroy on newer, if available. There are some other cleanups that may need to be done on older versions of node (such as prevent processing more data after destroying) For now, all tests are also successful on node 6.x

P.S.: Would be nice to open the issue before the PR, so we could discuss things like these, but thanks anyway for it! :wink:

dforz commented 5 years ago

We appreciate it, thank you.

apcurtiss commented 5 years ago

Thanks!