Copilot commented on code in PR #10:
URL:
https://github.com/apache/ignite-nodejs-thin-client/pull/10#discussion_r3542403431
##########
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();
+ });
Review Comment:
The detached `_finalizeResponse(...).catch(...)` disconnects on error, but
it does not reject the in-flight `request`. Because the request is removed from
`_requests` before finalization, `_disconnect()` will not reject it either, so
if `_finalizeResponse` ever throws (e.g. malformed frame, short buffer, or an
exception in affinity-topology handling), the caller can hang forever awaiting
this request. Reject the request in this catch before disconnecting.
--
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]
