wu-sheng commented on PR #141:
URL:
https://github.com/apache/skywalking-nodejs/pull/141#issuecomment-4898648384
## Design principle: this is a Node.js agent — be Node-native, don't port
Java 1:1
A general note for this PR and future `agent.core` work: **please don't
mechanically translate the Java agent's structure into TypeScript.** The Java
agent's shape is dictated by the JVM's threading model — background threads,
`ReentrantLock`/`synchronized`, `DataCarrier` blocking queues, generation
counters. Node.js is single-threaded with an event loop: there are no data
races, so there are no locks to port, and the platform already gives us most of
what those Java constructs exist to provide — `async`/`await`, `EventEmitter`,
grpc-js's **event-driven** connectivity API, native stream backpressure, and
OpenSSL for key parsing.
Where the Java agent is the **contract** (env var names, meter names,
wire/proto semantics, reconnect *policy*), match it. Where it's just
**implementation shaped by the JVM**, use the Node primitive instead.
### Why copying Java hurts (the issues it caused in this PR)
- **Locks/guards for a problem Node doesn't have.** Single-threaded ⇒ no
concurrent mutation ⇒ a ported `synchronized`/lock becomes an async re-entrancy
flag that only exists to prop up *other* ported machinery.
- **Extra timers = extra event-loop wakeups**, and they can keep the host
process alive. This is literally how the `watchConnectivityState` 24h deadline
regressed into a ref'd timer holding the process open (M1) — v1 used `Infinity`
and had no such timer.
- **Reimplementing platform primitives ⇒ more code, more bugs.** Multiple
defects in this PR trace directly to porting a Java idiom instead of using the
Node one (see table).
- **Dead abstractions ported ahead of need** become maintenance burden and
imply capability that isn't actually wired up.
### Examples in this PR
| Java-ism copied | Where | Issue | Node-native instead |
|---|---|---|---|
| `GRPCChannelManager.run()` background-thread **poll loop** +
`checkInFlight` lock + `reconnect`/`reconnectCount` state machine |
`GRPCChannelManager.ts` `boot()`/`runCheck()` | A 30s `setInterval` + lock is
armed for **every** agent, even the default single-backend case —
reimplementing connectivity detection that grpc-js already does event-driven.
v1 had none of this. | v1 was correct: event-driven
`channel.watchConnectivityState(state, Infinity, cb)` that re-arms itself;
grpc-js auto-reconnects a channel with backoff on its own. Arm a timer only
when it has real work — `isResolveDnsPeriodically` or `grpcServers.length > 1`.
|
| `watcherGeneration` monotonic counter to reject stale callbacks |
`GRPCChannelManager.ts:70` | A JVM generation-guard pattern duplicating a check
that already exists. | The closure's `this.managedChannel !== managed`
reference check (present since v1) is sufficient single-threaded. Delete the
counter. |
| Full **command framework**: `CommandService` queue + `drainScheduled` +
`setImmediate` drain + `CommandExecutorService` + `CommandSerialNumberCache` |
`commands/*` | `registerExecutor` is **never called**, so the executor map is
always empty and every command just flows through the queue → drain →
`warn('unsupported command')`. The entire pipeline is currently pure overhead.
| Node has no worker threads to hand off to — dispatch directly from
`receiveCommand`. Reintroduce structure only when real executors exist. |
| Serial-number dedup as `string[]` + O(n) `Array.includes` (Java bounded
queue) | `CommandSerialNumberCache.ts:38` | Linear scan + manual `shift()`
eviction to mimic an `ArrayBlockingQueue`. | `Set` — O(1) membership,
insertion-ordered so oldest-eviction is trivial. |
| PKCS#1→PKCS#8 ASN.1 hand-rewrap (Java `KeyFactory` requires PKCS#8) |
`PrivateKeyUtil` — **already removed in `ade5c3b`** ✅ | Java's constraint
doesn't exist in Node; the hand-built DER length prefix was also a latent
corruption risk. | Node/OpenSSL (`tls.createSecureContext`) accept PKCS#1 PEM
directly — this is the canonical example, and removing it was exactly right. |
The `PrivateKeyUtil` cleanup in the latest commit is the right instinct —
let's apply the same lens to the connectivity poll loop and the command
pipeline. Net: prefer grpc-js/EventEmitter/async primitives over ported
threads, locks, and queues; keep Java only where it defines the *behavior*, not
the *mechanism*.
--
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]
