wu-sheng commented on PR #141:
URL:
https://github.com/apache/skywalking-nodejs/pull/141#issuecomment-4898501036
## Re-review of `ade5c3b` (fix: address review, 13 items)
Re-checked each fix against the actual grpc-js 1.14.4 internals. **12 of 13
are correctly addressed** — and thanks for adding tests for the
`unhandledRejection`, `Infinity` connectivity-watch deadline, and
shutdown-during-`preloadTlsMaterials` leak paths:
- **[High] index.ts** unhandledRejection — ✅ `bootstrapPromise.catch(() =>
{})` (host no longer crashes; `whenReady()` still rejects for opt-in awaiters).
- **[Med] `Infinity` watch deadline** — ✅ no more ref'd 24h timer / silent
stop after 24h.
- **[Med] switchToServer leak** — ✅ `if (this.closed)` guards after the
await and after build; channel is closed on the race.
- **[Med] Windows key perms** — ✅ `process.platform !== 'win32'` guard.
- **[Low] port validation / bare-IPv6 parse / fallback-SNI (single-hostname
only)** — ✅.
- **[Low] PrivateKeyUtil** — ✅ hand-rolled PKCS#1→PKCS#8 conversion removed;
Node/OpenSSL accept PKCS#1 PEM directly.
- **Nits** (double `getHeapSpaceStatistics`, redundant `if/else`, dead
helpers, key-wipe comment) — ✅.
**One still open: [Low] the trace `drain` race** — the new
`Promise.race([drain, error, close])` subscribes to events grpc-js never emits
on a *client-streaming* writable (`makeClientStreamRequest` emits only
`metadata`/`status`; `reportStatus` clears the write buffer without firing the
pending write callback), so the infinite await is unchanged. Evidence + a
bounded-timeout fix are in the [inline
reply](https://github.com/apache/skywalking-nodejs/pull/141#discussion_r3532611856).
Not a blocker.
--
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]
