Copilot commented on code in PR #10:
URL:
https://github.com/apache/ignite-nodejs-thin-client/pull/10#discussion_r3405385459
##########
src/internal/ClientSocket.ts:
##########
@@ -296,15 +311,40 @@ export default class ClientSocket {
if (this._requests.has(requestId)) {
const request = this._requests.get(requestId);
this._requests.delete(requestId);
+
+ // Carve a fresh, independent MessageBuffer from just this
message's
+ // payload bytes (after length field + request-id). Passing a
copy
+ // rather than the shared socket buffer prevents two cursors
created
+ // from the same TCP segment from aliasing the same position
pointer
+ // and corrupting each other's reads under parallel scan
workloads.
+ // Built only on the matched-request path so unmatched frames
cost no copy.
+ const headerConsumed = isHandshake
+ ? BinaryUtils.getSize(BinaryUtils.TYPE_CODE.INTEGER)
// 4 B: length only
+ : BinaryUtils.getSize(BinaryUtils.TYPE_CODE.INTEGER) +
// 4 B: length
+ BinaryUtils.getSize(BinaryUtils.TYPE_CODE.LONG);
// 8 B: request-id
+ const freshBuffer = MessageBuffer.from(
+ buffer.getSlice(msgStart + headerConsumed, msgEnd),
+ 0
+ );
Review Comment:
`buffer.getSlice` appears to take `(start, length)` (as used earlier with
`buffer.getSlice(this._offset - length, length)`), but here it’s being called
with `(start, endOffset)` (`msgEnd`). This will likely slice the wrong number
of bytes (potentially too many) and can corrupt response parsing. Calculate the
payload length explicitly (e.g., `msgEnd - (msgStart + headerConsumed)` or
`length - headerConsumed`) and pass that as the second argument.
##########
src/internal/Logger.ts:
##########
@@ -41,4 +41,10 @@ export default class Logger {
console.log('ERROR: ' + data, ...args);
}
}
+
+ static logWarning(data: string, ...args: any[]) {
+ if (Logger._debug) {
+ console.log('WARNING: ' + data, ...args);
+ }
+ }
Review Comment:
For warning-level logs, `console.warn(...)` is generally preferred over
`console.log(...)` so warnings go to stderr and can be filtered/collected
separately in many runtimes. If you want consistent stream behavior, consider
documenting that choice; otherwise switching to `console.warn` would better
match the method’s intent.
##########
src/internal/ClientSocket.ts:
##########
@@ -296,15 +311,40 @@ export default class ClientSocket {
if (this._requests.has(requestId)) {
const request = this._requests.get(requestId);
this._requests.delete(requestId);
+
+ // Carve a fresh, independent MessageBuffer from just this
message's
+ // payload bytes (after length field + request-id). Passing a
copy
+ // rather than the shared socket buffer prevents two cursors
created
+ // from the same TCP segment from aliasing the same position
pointer
+ // and corrupting each other's reads under parallel scan
workloads.
+ // Built only on the matched-request path so unmatched frames
cost no copy.
+ const headerConsumed = isHandshake
+ ? BinaryUtils.getSize(BinaryUtils.TYPE_CODE.INTEGER)
// 4 B: length only
+ : BinaryUtils.getSize(BinaryUtils.TYPE_CODE.INTEGER) +
// 4 B: length
+ BinaryUtils.getSize(BinaryUtils.TYPE_CODE.LONG);
// 8 B: request-id
+ const freshBuffer = MessageBuffer.from(
+ buffer.getSlice(msgStart + headerConsumed, msgEnd),
+ 0
+ );
+
if (isHandshake) {
- await this._finalizeHandshake(buffer, request);
+ await this._finalizeHandshake(freshBuffer, request);
}
else {
- await this._finalizeResponse(buffer, request);
+ await this._finalizeResponse(freshBuffer, request);
}
}
else {
- throw IgniteClientError.internalError('Invalid response id: '
+ requestId);
+ // No pending request matches this response id. At the protocol
+ // version this client negotiates (<= 1.4.0) the server has no
reason
+ // to send an unsolicited frame: affinity-topology updates
ride on
+ // response flags (handled in _finalizeResponse), and
notification /
+ // heartbeat frames only exist in later protocol versions this
client
+ // does not speak. An unmatched id therefore most likely
indicates a
+ // parsing desync. Discard it so the connection survives
instead of
+ // crashing, but warn (when debug is enabled) so the anomaly is
+ // diagnosable rather than silently masked.
+ Logger.logWarning('Discarding response frame with unmatched
request id: ' + requestId);
Review Comment:
The comment indicates an unmatched request id is most likely a parsing
desync, but the code now discards the frame and keeps the connection alive. In
a desync scenario, continuing can lead to repeated misparsing and undefined
client behavior. Consider failing fast by setting `_error` and disconnecting on
the first unmatched frame (or after a small threshold), and/or logging at a
level that is visible even when debug is disabled so operators can detect and
act on this condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
