Fix BUG when receiving splited TCP message

[ciePIC]

Mainly

Sometimes modbus TCP message can be splited.
This cause the on "data" function in tcpport is call multiple time but never with full data.
With this fix, all received data are stored in a buffer and message is processed only when the buffer contain enougth data.
The quantity of requested data is simply the size of MBAP + the length of the message (extracted from the MBAP)

The Bug Was & the Fix

Context

I try to read 4 holding register from a device. this device is often spliting message.
The device 4 register is [0, 0, 0, 1]

The BUG

I have added (only for test) a console log in the on "data" function to log received data :

• On line 1 > New data is received and comple
• On line 2 > Message is parsed and the function return as expected [0, 0, 0, 1]
• On line 3 > Only the first part of message is received
• On line 4 > Incomplete Message is parsed and the function retrun no data []
• On line 5 > The rest data of the message
• You can see that line 3 + line 5 is pretty the content of line 1 (the complete message). only the request ID change

The Fix

I have added (only for test) 2 console log in the on "data" function :

• The first log when message is incomplete and skip
• The second log when message processing is executed
• Both log received data and the content of the new buffer

• On line 1 > New data is received and copy tp the new buffer. Not enougth data
• On line 2 > New data is received and append to the new buffer. Now, enougth data
• On line 3 > Message is parsed and the function return as expected [0, 0, 0, 1]
• The BUG is Fixed !

Summary by CodeRabbit

• Bug Fixes
  • Improved TCP message buffering and parsing: incoming network chunks are now accumulated and validated before parsing, ensuring complete Modbus TCP frames are processed reliably.
  • Prevents errors from fragmented packets by only consuming bytes for fully processed messages, preserves message identifiers, and clears the receive buffer when a connection closes.

[yaacov]

@ciePIC thank you for the pull request 💚
can you look at the linting issue?

@coderabbitai, please review

[coderabbitai]

The PR description images show the bug and fix clearly:

• Image 1 (working case): A complete TCP frame <Buffer 00 99 00 00 00 0b 01 03 08 ...> arrives in one chunk and is parsed correctly as [0, 0, 0, 1].
• Image 2 (the bug): The frame is split — first chunk <Buffer 00 01 00 00 00 01 03> arrives incomplete, gets parsed immediately and returns data: [], then the second chunk <Buffer 08 00 00 00 00 00 00 00 01> arrives too late.
• Image 3 (the fix): The first incomplete chunk is held (nOK) in the buffer; when the second chunk arrives it is appended (OK), the full reassembled frame is then parsed correctly as [0, 0, 0, 1].

---

@yaacov: Sure! I'll kick off a full review of this PR now, including checking for the linting issues.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 13e216d3-ee87-4676-801c-e8ee789a6334

📥 Commits

Reviewing files that changed from the base of the PR and between 597d123 and ffeb93b.

📒 Files selected for processing (1)

• ports/tcpport.js

🚧 Files skipped from review as they are similar to previous changes (1)

• ports/tcpport.js

---

📝 Walkthrough

Walkthrough

TcpPort now accumulates incoming socket data in a persistent _clientRcvData buffer, verifies complete Modbus TCP frames exist before parsing using length + MIN_MBAP_LENGTH, consumes parsed bytes from the persistent buffer, and resets the buffer on socket close.

Changes

Cohort / File(s)	Summary
TCP Receive Buffering ports/tcpport.js	Replace per-chunk parsing with persistent _clientRcvData accumulation; debug logs reference aggregated buffer; frame-parsing and MBAP length reads use the aggregated buffer; verify length + MIN_MBAP_LENGTH before parsing; consume parsed bytes by slicing the persistent buffer; clear buffer on socket close.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Socket as Socket<br/>Event
    participant TCPPort as TCP Port<br/>Handler
    participant Buffer as _clientRcvData<br/>Buffer
    participant Parser as Frame<br/>Parser

    rect rgba(100, 200, 100, 0.5)
    Note over Client,Parser: Buffered receive + parse flow
    Client->>Socket: send chunk 1
    Socket->>TCPPort: 'data' event
    TCPPort->>Buffer: append chunk 1
    TCPPort->>Parser: check buffer length
    Parser-->>TCPPort: incomplete
    TCPPort->>TCPPort: wait for more data

    Client->>Socket: send chunk 2
    Socket->>TCPPort: 'data' event
    TCPPort->>Buffer: append chunk 2
    TCPPort->>Parser: check buffer length
    Parser-->>TCPPort: complete frame found
    TCPPort->>Parser: parse frame
    Parser-->>TCPPort: frame extracted
    TCPPort->>TCPPort: emit frame
    TCPPort->>Buffer: slice consumed bytes
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Hop, hop! Chunks arrive and nest in line,
I tuck them in _clientRcvData just fine,
Counting MBAP, slicing bytes with care,
Frames leap out tidy — parsed and bare,
A rabbit cheers for clean, buffered design 🐇

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix BUG when receiving splited TCP message' directly describes the main change: buffering TCP data to handle split messages across multiple events.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

ports/tcpport.js (1)

154-163: Consider clearing receive buffer on close.

If the TcpPort instance were ever reused after close/reconnect, stale data in _clientRcvData could cause unexpected behavior. Clearing the buffer on close would make the state machine more robust.

💡 Optional: Clear buffer on close

         this._client.on("close", function(had_error) {
             if (self.openFlag)  {
                 self.openFlag = false;
+                self._clientRcvData = Buffer.alloc(0);
                 modbusSerialDebug("TCP port: signal close: " + had_error);
                 handleCallback(had_error);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ports/tcpport.js` around lines 154 - 163, When handling the socket "close" in
TcpPort, clear the receive buffer to avoid stale data on reconnect: inside the
this._client.on("close", function(had_error) { ... }) handler (the block that
checks self.openFlag and sets self.openFlag = false, calls handleCallback, emits
"close" and removeAllListeners), reset the instance receive buffer
_clientRcvData to an empty value (e.g., empty Buffer/string/array consistent
with how _clientRcvData is used) so the state machine starts clean on
reuse/reconnect.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ports/tcpport.js`:
- Line 111: In the callback where modbusSerialDebug is called, replace the
incorrect this._clientRcvData reference with the TcpPort instance reference
(self._clientRcvData) because inside function(data) this is the socket
(_client); update the modbusSerialDebug call in that callback to use
self._clientRcvData so the actual receive buffer is logged correctly (look for
the function(data) callback and the modbusSerialDebug invocation).
- Around line 99-100: The ESLint spaced-comment error is caused by the inline
comment on the Buffer initialization; update the initializer for
self._clientRcvData in ports/tcpport.js (the line that sets self._clientRcvData
= Buffer.alloc(0)) to ensure the comment has a space after the // (e.g., change
"//Initialize a variable..." to "// Initialize a variable...") so the linter
passes.

---

Nitpick comments:
In `@ports/tcpport.js`:
- Around line 154-163: When handling the socket "close" in TcpPort, clear the
receive buffer to avoid stale data on reconnect: inside the
this._client.on("close", function(had_error) { ... }) handler (the block that
checks self.openFlag and sets self.openFlag = false, calls handleCallback, emits
"close" and removeAllListeners), reset the instance receive buffer
_clientRcvData to an empty value (e.g., empty Buffer/string/array consistent
with how _clientRcvData is used) so the state machine starts clean on
reuse/reconnect.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e05c6de1-a0d5-46b2-97d3-5c7518a2aea6

📥 Commits

Reviewing files that changed from the base of the PR and between ee6da5a and 0806bc4.

📒 Files selected for processing (1)

• ports/tcpport.js

[ciePIC]

@ciePIC thank you for the pull request 💚 can you look at the linting issue?

@coderabbitai, please review

Hi @yaacov,
I just accept the fix and smart proposition of codeRabbit.
I have also do the sugestion of clearing the buffer on close event.

Did you need other thing ?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the current PR diff to verify linting and buffer clear on close are in place
gh pr view 602 --repo yaacov/node-modbus-serial --json files,state,reviews
gh pr diff 602 --repo yaacov/node-modbus-serial

Length of output: 7374

---

🏁 Script executed:

#!/bin/bash
# Check the actual file for linting issues
cat -A ports/tcpport.js | head -160 | tail -70
# Also check if there's an eslint config
fd -e json -e yaml -e yml -e js . -d 1 | grep -i eslint
cat .eslintrc* 2>/dev/null || cat .eslintrc.js 2>/dev/null || cat .eslintrc.json 2>/dev/null || true

Length of output: 11559

---

@ciePIC Great progress! The two originally flagged issues (the // Initialize comment spacing and the this._clientRcvData debug reference) are now fixed, and the buffer clear on close looks good. 🎉

However, there are still two no-trailing-spaces lint violations remaining (this rule is set to "error" in .eslintrc.json):

1. Trailing space after the semicolon on the Buffer.concat line:

-            self._clientRcvData = Buffer.concat([self._clientRcvData, data]); 
+            self._clientRcvData = Buffer.concat([self._clientRcvData, data]);

2. Trailing whitespace on the blank line immediately after it (the blank line has leftover indentation spaces):

-            
+

Both are in the this._client.on("data", ...) handler around lines 112–113. Simply removing the trailing spaces/whitespace on those two lines should make the linter pass.

You can catch these automatically in the future by running npm run lint (or npx eslint ports/tcpport.js) locally before pushing. Many editors also have a "trim trailing whitespace on save" option that helps prevent this.

[yaacov]

@ciePIC thank you for the fix
https://www.npmjs.com/package/modbus-serial/v/8.0.24 with the fix is out 🎉 🍰