Copilot commented on code in PR #10:
URL:
https://github.com/apache/ignite-nodejs-thin-client/pull/10#discussion_r3405368687
##########
src/internal/ClientSocket.ts:
##########
@@ -288,23 +299,50 @@ 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.getSlice(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);
+ // 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:
Discarding unmatched response frames can leave the client in an inconsistent
state: the real pending request may never be completed (potentially leaking
entries in `_requests` / hanging callers), and if this indicates a parsing
desync, continuing may cause subsequent frames to be mis-parsed. Consider
treating this as fatal (set `_error` + disconnect) like before, or (if you want
resiliency) explicitly fail/clear all pending requests and reset parsing state
after logging so callers don’t hang indefinitely.
##########
src/internal/ClientSocket.ts:
##########
@@ -288,23 +299,50 @@ 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.getSlice(msgStart + headerConsumed, msgEnd),
+ 0
+ );
Review Comment:
The comment says you’re passing a *copy*, but `buffer.getSlice(...)` often
implies a view/slice rather than a deep copy (depending on
`MessageBuffer.getSlice` implementation). Since the main goal is to avoid
sharing the `position` pointer (i.e., independent `MessageBuffer` instances),
either adjust the comment to reflect that, or ensure the bytes are actually
copied if that’s required for correctness.
##########
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 warnings, prefer `console.warn(...)` over `console.log(...)` so log
level filtering/formatting works as expected in many runtimes and log
collectors. This also aligns better with the semantics of a `logWarning` API.
##########
src/Cursor.ts:
##########
@@ -160,8 +175,15 @@ export abstract class BaseCursor<T> {
if (!this._buffer && this._hasNext) {
await this._getNext();
}
- await this._read(this._buffer)
- this._buffer = null;
+ if (this._buffer) {
+ await this._read(this._buffer);
+ this._buffer = null;
+ } else {
+ // No buffer and no next page — cursor is exhausted.
+ // Set _values to null so getValue() returns null and
+ // hasMore() returns false rather than replaying old entries.
+ this._values = null;
+ }
return this._values;
Review Comment:
Setting `_values = null` changes the observable return value of this method
(it now returns `null` when exhausted), which can be surprising for a method
that appears to return a collection. If the public/declared return type is
`T[]` (or callers expect an array), return an empty array instead; `getValue()`
can still return `null` naturally when `_valueIndex >= _values.length`, and
`hasMore()` will remain false. If `null` is intended, consider documenting it
explicitly in the method’s JSDoc/typing.
--
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]
