Re: [PR] Fix four bugs in ClientSocket, Cursor and Errors [ignite-nodejs-thin-client]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #10:
URL: https://github.com/apache/ignite-nodejs-thin-client/pull/10#discussion_r3520398418


##########
src/Cursor.ts:
##########
@@ -160,8 +180,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;
+        }

Review Comment:
   `_getValues()` is declared to return `Promise<T[]>`, but in the exhaustion branch it sets `_values = null` and returns it. This violates the method’s declared contract (and any generated `.d.ts`) and can cause runtime errors for callers that assume an array. Prefer returning an empty array (or change the return type/JSDoc to include `null`).



##########
src/internal/ClientSocket.ts:
##########
@@ -296,15 +316,66 @@ 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). getSlice() returns
+                // a view over the shared socket buffer, but MessageBuffer.from() copies
+                // those bytes (via Buffer.from), so freshBuffer owns an independent
+                // buffer with its own position pointer. That independence prevents two
+                // cursors created from the same TCP segment from aliasing the same
+                // position 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);
+                    // Handshake is single-in-flight, transitions _state and issues no
+                    // nested request, so it is safe to await inline on the queue.
+                    await this._finalizeHandshake(freshBuffer, request);
                 }
                 else {
-                    await this._finalizeResponse(buffer, request);
+                    // Do NOT await on the processing queue: a payloadReader may issue a
+                    // nested request on this same socket and await its reply (e.g.
+                    // GET_BINARY_TYPE when reading a COMPLEX_OBJECT whose type is not yet
+                    // cached in this client's BinaryTypeStorage). That reply arrives as a
+                    // later 'data' event chained behind this very queue entry, so awaiting
+                    // here would deadlock — the entry can only complete once the reply is
+                    // processed, but the reply can only be processed by a later entry.
+                    // freshBuffer is an independent copy of this message's payload, so
+                    // finalizing it off the parse chain cannot corrupt _buffer/_offset.
+                    // With finalize detached the CONNECTED path of _processResponse has no
+                    // remaining await and runs to completion synchronously, so two
+                    // invocations still cannot interleave on _buffer/_offset and the parse
+                    // race stays closed.
+                    this._finalizeResponse(freshBuffer, request).catch(err => {
+                        this._error = err.message;
+                        this._disconnect();
+                    });
                 }
             }
             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 never sends unsolicited
+                // frames: 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. Requests are also
+                // never removed while still awaiting a response (there is no client-side
+                // timeout), so an unmatched id cannot be a late or duplicate reply.
+                // It therefore means the response byte stream has desynced, after which
+                // every subsequent frame is garbage and the originating request would
+                // otherwise hang forever. Fail fast: throwing propagates to the socket
+                // 'data' handler's catch, which disconnects and rejects all pending
+                // requests with LostConnectionError so callers can recover instead of
+                // hanging.
+                throw IgniteClientError.internalError(
+                    'Response stream desync: received a frame with unmatched request id ' + requestId);

Review Comment:
   The PR description says unsolicited server notification/heartbeat frames (Ignite 2.14+) are logged and discarded, but this code still treats an unmatched `requestId` as a fatal stream desync and throws (disconnecting the socket). Please align the PR description with the implemented fail-fast behavior, or adjust this branch to actually discard known unsolicited frames if that’s required for the targeted protocol/server versions.



-- 
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: dev-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org