fix(nodejs): transport layer self-healing and logger close deferral - #1375
Conversation
- Endpoints: prefix gRPC target with resolver scheme (ipv4:/ipv6:/dns:), bracket IPv6 hosts for grpc-js - RpcClientManager: key rpc clients by endpoints facade string instead of object identity (endpoints are recreated on every route fetch, leaking duplicate channels); add evict() - RpcClient: add keepalive (5min/30s + permit_without_calls) and max message size channel options - BaseClient: rejoin isolated endpoints after successful heartbeat; rebuild transport (evict rpc client + refresh telemetry session) after consecutive heartbeat failures with a 30s per-endpoints cooldown; respond VerifyMessageResult (not echo command) to verify-message command; return real process diagnostics for print-thread-stack-trace - BaseClient: defer logger.close() to the next macrotask so subclass shutdown logs do not throw 'log stream had been close'
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Comprehensive transport layer self-healing and several correctness fixes for the Node.js client. Well-structured PR with clear improvements:
- gRPC target scheme prefix — fixes multi-address and IPv6 resolution
- RpcClientManager string-keying — fixes channel leak from endpoint object identity
- Keepalive config — aligns with Java client (5min/30s)
- Heartbeat self-healing — evicts stale transport after consecutive failures with proper throttling
- VerifyMessage protocol fix — responds with VerifyMessageResult instead of echoing command
- Process diagnostics — real Node.js diagnostics instead of mock stack trace
- Logger close deferral — fixes 'log stream had been closed' error
- Endpoint tests — good coverage for the getGrpcTarget changes
All changes align with the Java client behavior and improve cross-language consistency. CI is green.
I've left a few inline comments with observations. Overall this is solid work.
Note: This PR and #1374/#1376 all add the same
tsconfig.test.json. Consider merge coordination to avoid conflicts.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [WARNING]
nodejs/src/rpc/RpcClientManager.ts:38— Keying byendpoints.facadestring instead of object identity is the correct fix for the channel leak. However, sincefacadeis derived from the endpoints' address list, two logically differentEndpointsinstances with the same addresses will now share a channel. This is the intended behavior (route refreshes create new objects with same addresses), but worth a brief comment in the code explaining why string-keying is correct here. (line outside diff)
| }).join(','); | ||
| switch (this.scheme) { | ||
| case AddressScheme.IPV4: | ||
| return `ipv4:${targets}`; |
There was a problem hiding this comment.
Good fix — adding the resolver scheme prefix (ipv4: / ipv6: / dns:) ensures grpc-js correctly handles multi-address and bare-IPv6 targets. This aligns with the Java client's Endpoints.grpcTarget() behavior. One thing to verify: ensure the RocketMQ Proxy's gRPC server accepts targets with these scheme prefixes. Standard grpc-js resolvers should handle them transparently, but worth confirming in integration tests.
| /** | ||
| * Build Node.js process diagnostics in place of Java-style thread stack traces. | ||
| * Java sends per-thread stacks via ThreadMXBean; Node.js is single-threaded per | ||
| * process, so we expose the closest equivalent runtime snapshot. |
There was a problem hiding this comment.
_getActiveHandles() is an undocumented Node.js internal API. The try/catch fallback is good defensive coding. Since this is best-effort diagnostics (not on a critical path), this is acceptable — just be aware it could break silently in future Node.js versions without notice.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Comprehensive transport layer self-healing and correctness fixes for the Node.js client. This is a significant reliability improvement.
Key changes reviewed:
-
Heartbeat transport self-healing — After 2 consecutive heartbeat failures, the transport layer is rebuilt: stale RpcClient is evicted, telemetry session is released and re-synced. Throttled by 30s cooldown to prevent recovery storms. Well-designed with proper error handling.
-
Logger close deferral —
setImmediate()defers logger close to the next macrotask, preventing "log stream had been closed" errors when subclass shutdown emits final log lines aftersuper.shutdown(). Clean fix. -
VerifyMessageResult fix — Now responds with
VerifyMessageResultcarrying the nonce, mirroring JavaBaseClient#onVerifyMessageCommand. Correct protocol alignment. -
PrintThreadStackTrace — Replaced mock stack trace with real Node.js process diagnostics. Good adaptation of Java concept to Node.js runtime.
-
gRPC channel options — Keepalive and message size limits now match Java defaults. Important for production stability.
-
Isolated endpoints rejoin — Successful heartbeat now removes endpoints from isolation set, mirroring Java
ClientImpl#doHeartbeat.
All changes are well-structured with appropriate error handling and logging. LGTM.
Automated review by github-manager-bot
…nto fix/nodejs-transport-self-healing # Conflicts: # nodejs/src/client/RpcClient.ts
P3 fix(nodejs): transport layer self-healing and logger close deferral
Endpoints.getGrpcTarget(): prefix the gRPC target with the resolver scheme —ipv4:/ipv6:(bracketed IPv6 hosts) /dns:— so grpc-js resolves multi-addressand bare-IPv6 targets correctly.
RpcClientManager: key rpc clients by theendpoints.facadestring instead ofobject identity (endpoints are recreated on every route fetch, leaking duplicate
channels); add
evict().RpcClient: keepalive (5 min / 30 s +keepalive_permit_without_calls) and maxmessage size channel options.
BaseClientheartbeat: rejoin isolated endpoints after success; after ≥ 2 consecutivefailures rebuild the transport (evict stale
RpcClient+ refresh telemetry session),throttled by a 30 s per-endpoints cooldown.
VerifyMessageResult(nonce) instead of echoingthe command.
logger.close()deferred to the next macrotask (setImmediate) so subclass shutdownlogs no longer throw
log stream had been closed.Tests:
nodejs/test/route/Endpoints.test.ts(4 cases, offline, all pass)