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

MIDParser silently errors because _destroy() is a no-op #33

Open ferm10n opened 2 years ago

ferm10n commented 2 years ago

I'm discovering that if there's an error during parsing, MidParser does not emit it, and the stream is silently destroyed. This might be caused by something I did while tinkering with the project, but it'd really help if I had some context about this section:

https://github.com/st-one-io/node-open-protocol/blob/f514d00d5ddc7b0d84f222c2e481e2a07cf16c91/src/MIDParser.js#L71-L73

It seems to be responsible for why the error gets eaten. If I remove it, the default handler emits the error event correctly.

How did earlier versions of node behave that prompted _destroy() to become a noop? I'd like to fix this without breaking backwards compat, but can't do that without more history on why it was added.

(I'm using node v14 btw)

ferm10n commented 2 years ago

Additionally, I think that SessionControlClient should be modified to emit an error when calling close() with an error.

I tried removing the _destroy() noop mentioned above, but still didn't get an error.

Here's a sample output with `NODE_DEBUG=open-protocol`, with the changes mentioned ``` OPEN-PROTOCOL 24875: new MIDParser _transform { mid: 61, revision: 1, noAck: false, stationID: 1, spindleID: 1, sequenceNumber: 1, messageParts: 1, messageNumber: 1, payload: } OPEN-PROTOCOL 24875: new MIDParser _transform err-parser { mid: 61, revision: 1, noAck: false, stationID: 1, spindleID: 1, sequenceNumber: 1, messageParts: 1, messageNumber: 1, payload: } Error: [Parser MID61] invalid key, mid: 61, parameter: channelID, expect: 2, receiver: 10 payload: {"cellID":102} at Object.parse (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/helpers.js:606:15) at Object.parser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/mid/0061.js:235:35) at MIDParser._transform (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:44:29) at MIDParser.Transform._read (internal/streams/transform.js:205:10) at MIDParser.Transform._write (internal/streams/transform.js:193:12) at writeOrBuffer (internal/streams/writable.js:358:12) at MIDParser.Writable.write (internal/streams/writable.js:303:10) at LinkLayer._onDataOpParser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:245:40) at OpenProtocolParser. (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:98:49) at OpenProtocolParser.emit (events.js:400:28) OPEN-PROTOCOL 24875: LinkLayer _onErrorParser Error: Error on parser [Error: [Parser MID61] invalid key, mid: 61, parameter: channelID, expect: 2, receiver: 10 payload: {"cellID":102}] at /home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:48:24 at Object.parser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/mid/0061.js:325:5) at MIDParser._transform (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:44:29) at MIDParser.Transform._read (internal/streams/transform.js:205:10) at MIDParser.Transform._write (internal/streams/transform.js:193:12) at writeOrBuffer (internal/streams/writable.js:358:12) at MIDParser.Writable.write (internal/streams/writable.js:303:10) at LinkLayer._onDataOpParser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:245:40) at OpenProtocolParser. (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:98:49) at OpenProtocolParser.emit (events.js:400:28) OPEN-PROTOCOL 24875: SessionControlClient _onErrorLinkLayer Error: Error on parser [Error: [Parser MID61] invalid key, mid: 61, parameter: channelID, expect: 2, receiver: 10 payload: {"cellID":102}] at /home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:48:24 at Object.parser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/mid/0061.js:325:5) at MIDParser._transform (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:44:29) at MIDParser.Transform._read (internal/streams/transform.js:205:10) at MIDParser.Transform._write (internal/streams/transform.js:193:12) at writeOrBuffer (internal/streams/writable.js:358:12) at MIDParser.Writable.write (internal/streams/writable.js:303:10) at LinkLayer._onDataOpParser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:245:40) at OpenProtocolParser. (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:98:49) at OpenProtocolParser.emit (events.js:400:28) OPEN-PROTOCOL 24875: SessionControlClient close Error: Error on parser [Error: [Parser MID61] invalid key, mid: 61, parameter: channelID, expect: 2, receiver: 10 payload: {"cellID":102}] at /home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:48:24 at Object.parser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/mid/0061.js:325:5) at MIDParser._transform (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/MIDParser.js:44:29) at MIDParser.Transform._read (internal/streams/transform.js:205:10) at MIDParser.Transform._write (internal/streams/transform.js:193:12) at writeOrBuffer (internal/streams/writable.js:358:12) at MIDParser.Writable.write (internal/streams/writable.js:303:10) at LinkLayer._onDataOpParser (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:245:40) at OpenProtocolParser. (/home/jsanders/Documents/redviking/tmp-worktree/node-open-protocol/src/linkLayer.js:98:49) at OpenProtocolParser.emit (events.js:400:28) OPEN-PROTOCOL 24875: LinkLayer _destroy ``` No error is ever emitted by the SessionControlClient.

Adding an emit('error') is easy enough in the close() method before emit('close') but does it makes sense? Would appreciate some input.