Fix bugs found in codebase-wide review, with regression tests#19
Merged
Conversation
Core infrastructure: - utils.js: CallbackPromise now observes rejections from async executors instead of discarding them, so throws inside async resolvers reject the promise rather than leaving it pending forever with an unhandledRejection; InvertKeyValues Map branch no longer relies on Iterator.prototype.forEach (missing on Node 20, the minimum supported engine) - Layer.js: context timeouts now remove their entry from __contextToCallbackTimeouts (previously leaked one entry per timed-out request); clearContexts materializes entries before clearing so callers no longer receive an always-empty live iterator - MultiplexLayer: layerContext called incrementContext(this) from a plain module function where `this` is undefined, so every multiplexed send threw a TypeError; duplicate disconnect() now returns after resolving - TCP layer: the socket 'error' handler never settled the connect promise, so a refused connection hung connected() forever; disconnect() during Connecting now destroys the in-flight socket instead of leaking the established connection; Scan never initiated a connection and therefore never yielded results; connected() resolves false when no connect was ever started - UDP layer: handleDestroy now closes the bound socket instead of only unref'ing it (the port stayed bound and receiving for the process life) Modbus: - PDU.Decode validates wire length fields (byte counts, exception codes, write echoes) against the received data; malformed frames now produce a decodable error that rejects the pending request instead of throwing an uncaught ERR_OUT_OF_RANGE from the socket data handler - readCoils/readDiscreteInputs unpack the packed bytes into the requested number of coil statuses (LSB-first) instead of resolving raw bytes with undefined pad bits - requests now arm a timeout (options.timeout, default 10s) so a dropped response rejects instead of pending forever and leaking its context - per-request unitID/protocolID use ?? instead of ||, so unitID 0 (broadcast) is no longer silently replaced by the default - constants: SerialLineFunctions renamed (deprecated misspelled alias retained) and merged into FunctionNames so serial-line responses no longer decode as 'Unknown' CIP core: - object.js: OptionalServiceList is class attribute 5 per CIP Vol 1 (was duplicated with OptionalAttributeList at 4); GetAttributeSingle fallback referenced CommonClassAttribute.getCode which does not exist (group object intended) - Identity: Status.configured reads bit 2 per CIP Vol 1 Table 5-2.3 (bit 3 is reserved and always decoded 0 on compliant devices) - datatypes: STRINGN honors the decoded character width instead of always decoding UTF-16LE; BOOL reads enough bytes to contain the addressed bit (positions 8+ always decoded false before); ARRAY/ABBREV_ARRAY EncodeSize sums per-element sizes so variable-size item types no longer throw mid-write or corrupt the stream - EPATH datatype segment: ARRAY encodeSize accounts for 1/2/4-byte bounds to match encodeTo (bounds > 255 threw a RangeError mid-encode) - service.js and Logix5000 objects/datatypes modules: add .js extensions to ESM imports (these modules threw ERR_MODULE_NOT_FOUND when imported); __shared.js exports SymbolType by name as symbol.js expects EIP / Logix5000 / PCCC: - CIPInternalLayer/CIPConnectionLayer: pass the required offsetRef to request.response(); every PCCC-over-CIP response previously threw a TypeError so no forwarded request could ever complete - CIPInternalLayer.exploreAttributes queries through maxAttribute inclusive (loop stopped one short) - EIP: connect() queues callbacks instead of overwriting, so a second connect while RegisterSession is in flight no longer silently drops the first caller's callback; host strings without a port parse correctly - Logix5000: getSymbolInstanceID strips member access from dotted tags (the stripping line was commented out, so writeTag on any dotted tag hung); writeTag rejects cleanly for unknown tags (the previous guard could never fire and the resulting TypeError was swallowed); readTemplateClassAttributes wraps its body so protocol errors reject; readTemplate clamps the per-fragment remaining-bytes field to 16 bits for definitions larger than 64KB - remove empty src/core/cipmodbus module; fix latent offset/this bugs inside the commented-out CIP Modbus layer Every fix is covered by a regression test that fails against the previous code (verified: 39 of the new/updated tests fail on the unfixed sources). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NzFBvKrXRNpxmUpeFLeWWF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Core infrastructure:
instead of discarding them, so throws inside async resolvers reject the
promise rather than leaving it pending forever with an unhandledRejection;
InvertKeyValues Map branch no longer relies on Iterator.prototype.forEach
(missing on Node 20, the minimum supported engine)
__contextToCallbackTimeouts (previously leaked one entry per timed-out
request); clearContexts materializes entries before clearing so callers
no longer receive an always-empty live iterator
module function where
thisis undefined, so every multiplexed sendthrew a TypeError; duplicate disconnect() now returns after resolving
so a refused connection hung connected() forever; disconnect() during
Connecting now destroys the in-flight socket instead of leaking the
established connection; Scan never initiated a connection and therefore
never yielded results; connected() resolves false when no connect was
ever started
unref'ing it (the port stayed bound and receiving for the process life)
Modbus:
write echoes) against the received data; malformed frames now produce a
decodable error that rejects the pending request instead of throwing an
uncaught ERR_OUT_OF_RANGE from the socket data handler
number of coil statuses (LSB-first) instead of resolving raw bytes with
undefined pad bits
response rejects instead of pending forever and leaking its context
(broadcast) is no longer silently replaced by the default
retained) and merged into FunctionNames so serial-line responses no
longer decode as 'Unknown'
CIP core:
duplicated with OptionalAttributeList at 4); GetAttributeSingle fallback
referenced CommonClassAttribute.getCode which does not exist (group
object intended)
is reserved and always decoded 0 on compliant devices)
decoding UTF-16LE; BOOL reads enough bytes to contain the addressed bit
(positions 8+ always decoded false before); ARRAY/ABBREV_ARRAY
EncodeSize sums per-element sizes so variable-size item types no longer
throw mid-write or corrupt the stream
to match encodeTo (bounds > 255 threw a RangeError mid-encode)
to ESM imports (these modules threw ERR_MODULE_NOT_FOUND when imported);
__shared.js exports SymbolType by name as symbol.js expects
EIP / Logix5000 / PCCC:
request.response(); every PCCC-over-CIP response previously threw a
TypeError so no forwarded request could ever complete
inclusive (loop stopped one short)
connect while RegisterSession is in flight no longer silently drops the
first caller's callback; host strings without a port parse correctly
(the stripping line was commented out, so writeTag on any dotted tag
hung); writeTag rejects cleanly for unknown tags (the previous guard
could never fire and the resulting TypeError was swallowed);
readTemplateClassAttributes wraps its body so protocol errors reject;
readTemplate clamps the per-fragment remaining-bytes field to 16 bits
for definitions larger than 64KB
inside the commented-out CIP Modbus layer
Every fix is covered by a regression test that fails against the previous
code (verified: 39 of the new/updated tests fail on the unfixed sources).