Copilot commented on code in PR #10:
URL:
https://github.com/apache/ignite-nodejs-thin-client/pull/10#discussion_r3405280207
##########
src/internal/ClientSocket.ts:
##########
@@ -288,23 +299,44 @@ export default class ClientSocket {
this._logMessage(requestId, false, buffer.getSlice(this._offset -
length, length));
+ // Record boundaries before the socket buffer is potentially
cleared.
+ const msgEnd = this._offset;
+ const msgStart = msgEnd - length;
+
if (this._offset === buffer.length) {
this._buffer = null;
this._offset = 0;
}
+ // 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.
+ 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.buffer.slice(msgStart + headerConsumed, msgEnd),
+ 0
+ );
Review Comment:
`buffer.buffer` is an `ArrayBuffer` (and may require `byteOffset`
adjustments when the underlying Node.js `Buffer` is a view). Slicing it with
`msgStart/msgEnd` indices risks extracting the wrong bytes (and may hand
`MessageBuffer.from` an unexpected type), which can corrupt response parsing.
Prefer slicing the actual byte sequence (e.g., via the underlying `Buffer`
slice API or an existing `MessageBuffer` slicing helper) so offsets account for
`byteOffset` and the result type matches what `MessageBuffer.from` expects.
##########
src/internal/ClientSocket.ts:
##########
@@ -288,23 +299,44 @@ export default class ClientSocket {
this._logMessage(requestId, false, buffer.getSlice(this._offset -
length, length));
+ // Record boundaries before the socket buffer is potentially
cleared.
+ const msgEnd = this._offset;
+ const msgStart = msgEnd - length;
+
if (this._offset === buffer.length) {
this._buffer = null;
this._offset = 0;
}
+ // 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.
+ 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.buffer.slice(msgStart + headerConsumed, msgEnd),
+ 0
+ );
+
if (this._requests.has(requestId)) {
const request = this._requests.get(requestId);
this._requests.delete(requestId);
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);
+ // Ignite 2.14+ sends unsolicited server-side notification
frames
+ // (topology changes, heartbeats) whose request IDs do not
match any
+ // pending client request. Silently discard them and continue
reading.
+ Logger.logDebug('Discarding server notification frame with id:
' + requestId);
Review Comment:
Silently discarding any frame with an unknown `requestId` can mask real
protocol/desync bugs (e.g., a client-side request tracking issue) and make
failures much harder to diagnose. Consider narrowing this behavior to known
notification frame types (or gating it by protocol version/feature negotiation)
and keeping a stricter fallback (e.g., disconnect or raise an internal error)
for truly unexpected frames.
--
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]
