T1B0 commented on code in PR #4552:
URL: https://github.com/apache/iggy/pull/4552#discussion_r4342471827
##########
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:
the whole issue is here: emit('end') should not be inside a promise chain -
which may reject - we should settle them then emit end from outside of it
##########
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:
this is per socket ^^ CommandResponseStream.drive() re-dials on every
command retry with timeout=30s by default => it would emit "connecting" event
on failed connection with retry too ! so a single command against a failed
server/port would probably emit 60+ "connecting" events
##########
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:
destroy becoming async is a breaking api change. what happen if that promise
reject ? caller would have to watch for this. actual stream implem and e2e test
use a sync destroy and have not been updated by this PR. unhandled rejection of
this promise would terminate current process.
##########
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:
worth mentioning: getClient() factory return a simpleClient without those
on/once method
--
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]
