Arham-Begani commented on code in PR #4552:
URL: https://github.com/apache/iggy/pull/4552#discussion_r4347129640
##########
foreign/node/src/client/client.socket.ts:
##########
@@ -971,14 +988,18 @@ export class CommandResponseStream extends EventEmitter {
return connection.socket;
}
- destroy(): void {
+ destroy(): Promise<void> {
Review Comment:
Fixed in a3de19bd and 63c8f036. `CommandResponseStream.destroy()` and
`getRawClient().destroy()` are synchronous again and return `void`. The
high-level `Client` owns the completion promise and observes `end` from
construction, so closing the raw client first cannot leave a later
`Client.destroy()` hanging. Regression tests cover the raw return value,
repeated destruction and that raw-first path.
Code, tests and this response were prepared with AI assistance, as disclosed
in the PR.
##########
foreign/node/src/client/client.socket.ts:
##########
@@ -971,14 +988,18 @@ export class CommandResponseStream extends EventEmitter {
return connection.socket;
}
- destroy(): void {
+ destroy(): Promise<void> {
+ if (this.destroyPromise) return this.destroyPromise;
+ const closed = [...this.connections].map((connection) =>
connection.socketClosed);
+ this.destroyPromise = Promise.all(closed).then(() => { this.emit('end');
});
Review Comment:
Fixed in a3de19bd. Socket closures are collected with `Promise.allSettled`,
then `end` is scheduled with `process.nextTick` outside the promise callback. A
subprocess regression verifies that a throwing `end` listener reaches
`uncaughtException`, rather than becoming an unhandled promise rejection; the
closure tests still verify that `end` waits for the sockets.
Code, tests and this response were prepared with AI assistance, as disclosed
in the PR.
##########
foreign/node/src/client/client.connection.ts:
##########
@@ -147,10 +151,17 @@ export class IggyConnection extends EventEmitter {
signal?.throwIfAborted();
const socket = getTransport(this.config);
this.socket = socket;
+ process.nextTick(() => {
+ if (!this.ending && !this.failed)
+ this.emit('connecting');
Review Comment:
Fixed in a3de19bd. `CommandResponseStream` now retains a shared connecting
flag across failed dials and command retries, clearing it when a socket
connects. The regression issues commands against a closed port, verifies one
`connecting` event across retries, then checks recovery and the next connection
phase. The README now describes that behavior.
Code, tests and this response were prepared with AI assistance, as disclosed
in the PR.
##########
foreign/node/README.md:
##########
@@ -231,6 +231,34 @@ const client = new Client({
const stats = await client.system.getStats();
```
+### Connection events
+
+`Client` and `SingleClient` report their connection lifecycle through
Node.js-style `on`, `once`,
+`off` and `removeListener` methods. Each method returns the client, so calls
+can be chained. `SimpleClient` wraps an arbitrary raw client and does not
expose these methods.
Review Comment:
Added the explicit `getClient()` / `SimpleClient` caveat in a3de19bd. The
same section distinguishes synchronous raw destruction from the high-level
client's completion promise. The final source at 63c8f036 passed [Ubuntu
validation](https://github.com/Arham-Begani/Arham-Begani/actions/runs/39724500360):
579 unit tests on each of Node 23/24, lint/build, all nine applicable hooks,
73 TCP tests, eight TLS tests and four BDD scenarios. Local Windows build,
ESLint and 16 lifecycle regressions also passed. Fresh upstream workflows await
authorization; these hosted results are separate from upstream CI.
Code, tests and this response were prepared with AI assistance, as disclosed
in the PR.
--
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]
