Copilot commented on code in PR #10:
URL:
https://github.com/apache/ignite-nodejs-thin-client/pull/10#discussion_r3405296321
##########
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);
Review Comment:
A `freshBuffer` is allocated for every parsed frame, even when `requestId`
isn't present in `_requests` (e.g., unsolicited notifications). To reduce
avoidable allocations and copying, construct `freshBuffer` only inside the `if
(this._requests.has(requestId))` branch (or lazily when needed).
##########
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
Review Comment:
The comment says the payload bytes are passed as a 'copy', but `slice(...)`
may produce a view (sharing underlying memory) depending on the underlying
buffer type. Either update the comment to describe what is actually happening
(independent `MessageBuffer` state vs copied bytes), or use an explicit copy
operation if the intent is to duplicate the bytes.
##########
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:
The comment says the payload bytes are passed as a 'copy', but `slice(...)`
may produce a view (sharing underlying memory) depending on the underlying
buffer type. Either update the comment to describe what is actually happening
(independent `MessageBuffer` state vs copied bytes), or use an explicit copy
operation if the intent is to duplicate the bytes.
##########
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:
The client now discards *all* frames with unknown `requestId`s. If an
unexpected/invalid response-id occurs (protocol mismatch, client bookkeeping
bug, etc.), this can mask the error and potentially leave the originating
client operation waiting indefinitely. Consider only discarding frames that can
be positively identified as notifications (e.g., by parsing a notification
type/opcode), and otherwise failing fast (disconnect + reject pending requests)
or at least surfacing a warning/error and terminating the connection.
--
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]
