Skip to content

Commit 140285d

Browse files
ottomataottomata
committed
Refactor KafkaSSE disconnect handling
Call disconnect only on the first HTTP end event. The HTTP connection can be ended by one of several events from either the ClientRequest or the ServerResponse. Listen for all of these events, but only call KafkaSSE disconnect once. Calling disconnect multiple times doesn't hurt, but it does make debug logs noisy and look like something is going wrong. Also parallelize the clean up of the SSE + HTTP ServerResponse and the disconnection of the Kafka client. Before this patch, disconecting the Kafka client waited until the HTTP SSE and ServerResponse end() Promise resolved. Usnig Promise.all here allows both to be closed independently of each other. Also clean up some logging and comments along the way.
1 parent 7f55e2d commit 140285d

File tree

2 files changed

+106
-84
lines changed

2 files changed

+106
-84
lines changed

‎lib/KafkaSSE.js‎

Lines changed: 105 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -235,40 +235,54 @@ class KafkaSSE {
235235
mandatoryKafkaConfig
236236
);
237237

238-
// Call this.disconnect() when the http client request or response ends.
239-
this.req.on('abort', () => {
240-
this.log.debug('HTTP request abort, calling KafkaSSE disconnect.');
241-
this.disconnect();
242-
});
243-
this.req.on('aborted', () => {
244-
this.log.debug('HTTP request aborted, calling KafkaSSE disconnect.');
245-
this.disconnect();
246-
});
247-
this.req.on('close', () => {
248-
this.log.debug('HTTP request closed, calling KafkaSSE disconnect.');
249-
this.disconnect();
250-
});
251-
this.req.on('error', (e) => {
252-
this.log.debug(
238+
// Set up HTTP request and response end handlers.
239+
// We want to call this.disconnect() in response to any of these
240+
// events. We only need to call this.disconnect() the first time
241+
// one of these happens, so disconnect() will call
242+
// this.removeHttpEndListners to remove all of these listeners
243+
// as soon as disconnect() is called.
244+
const onceReqAbort = () => this.disconnect('HTTP request abort');
245+
const onceReqAborted = () => this.disconnect('HTTP request aborted');
246+
const onceReqClose = () => this.disconnect('HTTP request close');
247+
const onceReqError = (e) => {
248+
this.log.error(
253249
{err: e}, 'HTTP request encountered an error, calling KafkaSSE disconnect.'
254250
);
255-
this.disconnect(e);
256-
});
257-
258-
this.res.on('finish', () => {
259-
this.log.debug('HTTP response finished, calling KafkaSSE disconnect.');
260-
this.disconnect();
261-
});
262-
this.res.on('close', () => {
263-
this.log.debug('HTTP response closed, calling KafkaSSE disconnect.');
264-
this.disconnect();
265-
});
266-
this.res.on('error', (e) => {
267-
this.log.debug(
251+
this.disconnect('HTTP request error');
252+
};
253+
const onceResFinish = () => this.disconnect('HTTP response finish');
254+
const onceResClose = () => this.disconnect('HTTP response close');
255+
const onceResError = (e) => {
256+
this.log.error(
268257
{err: e}, 'HTTP response encountered an error, calling KafkaSSE disconnect.'
269258
);
270-
this.disconnect(e);
271-
});
259+
this.disconnect('HTTP response error');
260+
};
261+
262+
this.req.once('abort', onceReqAbort);
263+
this.req.once('aborted', onceReqAborted);
264+
this.req.once('close', onceReqClose);
265+
this.req.once('error', onceReqError);
266+
this.res.once('finish', onceResFinish);
267+
this.res.once('close', onceResClose);
268+
this.res.once('error', onceResError);
269+
270+
// Will be called theh first time disconnect() is called to keep it
271+
// from being called multiple times as the connection closes.
272+
this.removeHttpEndListeners = () => {
273+
this.log.debug('Removing all HTTP end listeners.');
274+
if (this.req) {
275+
this.req.removeListener('abort', onceReqAbort);
276+
this.req.removeListener('aborted', onceReqAborted);
277+
this.req.removeListener('close', onceReqClose);
278+
this.req.removeListener('error', onceReqError);
279+
}
280+
if (this.res) {
281+
this.res.removeListener('finish', onceResFinish);
282+
this.res.removeListener('close', onceResClose);
283+
this.res.removeListener('error', onceResError);
284+
}
285+
};
272286
}
273287

274288

@@ -278,7 +292,7 @@ class KafkaSSE {
278292
* is created and the consume loop starts. Up until _start is called,
279293
* it is possible to end the request with a sensable HTTP error response.
280294
* Once _start is called, a 200 response header will be written (via
281-
* sseClient.initialize()), and any further errors must be reported to the
295+
* sse.start()), and any further errors must be reported to the
282296
* client by emitting an error SSE event.
283297
*
284298
* Upon encountering any error, this.connectErrorHandler will be called.
@@ -346,17 +360,21 @@ class KafkaSSE {
346360
e = this._error(e);
347361
this.connectErrorHandler(e);
348362
})
349-
// Close KafkaConsumer, and either this.sse or this.res http response.
350363
.finally(() => {
351-
this.log.debug(
352-
'Finally finished consume loop and error handling. Calling KafkaSSE disconnect.'
353-
);
354-
this.disconnect();
364+
// Just in case we get here for a reason other than the HTTP request closing
365+
// or error handling, make sure we disconnect everything properly,
366+
// especially the KafkaConsumer.
367+
if (!this.is_finished) {
368+
return this.disconnect('Finally finished consume loop and error handling');
369+
}
355370
});
356371

357-
// wait for the `done` event to return
372+
// connect() will resolve after the KafkaSSE `done` event is fired by disconnect().
358373
return new P((resolve, reject) => {
359-
this._eventEmitter.on('done', resolve);
374+
this._eventEmitter.on('done', () => {
375+
this.log.info('KafkaSSE connection done.');
376+
resolve();
377+
});
360378
});
361379
}
362380

@@ -434,12 +452,10 @@ class KafkaSSE {
434452
`Registering Kafka event ${event} to be handled by ` +
435453
`function ${this.options.kafkaEventHandlers[event].name}`
436454
);
437-
consumer.on(event, this.options.kafkaEventHandlers[event]);
455+
this.kafkaConsumer.on(event, this.options.kafkaEventHandlers[event]);
438456
});
439457
}
440458

441-
// Save our consumer.
442-
// this.kafkaConsumer = consumer;
443459
return this.kafkaConsumer;
444460
})
445461

@@ -547,7 +563,7 @@ class KafkaSSE {
547563

548564
_start() {
549565
// Start the consume -> sse send loop.
550-
this.log.info('Initializing sseClient and starting consume loop.');
566+
this.log.info('Initializing KafkaSSE connection and starting consume loop.');
551567

552568
const responseHeaders = {};
553569
let disableSSEFormatting = false;
@@ -563,7 +579,7 @@ class KafkaSSE {
563579
}
564580
}
565581

566-
// Initialize the sse response and start sending
582+
// Initialize the SSEResponse and start sending
567583
// the response in chunked transfer encoding.
568584
this.sse = new SSEResponse(this.res, {
569585
headers: responseHeaders,
@@ -587,10 +603,10 @@ class KafkaSSE {
587603
return this._consume()
588604

589605
.then((kafkaMessage) => {
590-
// If the request is finished (by calling this.disconnect()),
606+
// If the request is finished (something called this.disconnect()),
591607
// then exit the consume loop now by returning a resolved promise.
592-
if (this.is_finished || !this.sse || this._resFinished()) {
593-
this.log.info('KafkaSSE connection finished. Returning from consume loop.');
608+
if (this.is_finished) {
609+
this.log.debug('KafkaSSE connection finished. Returning from consume loop.');
594610
return P.resolve();
595611
}
596612
if (!kafkaMessage || !kafkaMessage.message || !kafkaMessage.topic) {
@@ -667,7 +683,7 @@ class KafkaSSE {
667683
_consume() {
668684
// If we have finished (by calling this.disconnect),
669685
// don't try to consume anything.
670-
if (this.is_finished || this._resFinished()) {
686+
if (this.is_finished) {
671687
this.log.debug('KafkaSSE connection finished. Not attempting consume.');
672688
return P.resolve();
673689
}
@@ -842,45 +858,45 @@ class KafkaSSE {
842858
return false;
843859
}
844860

861+
845862
/**
846-
* Disconnects the KafkaConsumer and closes the sse client or http response.
863+
* Disconnects the KafkaConsumer and closes the sse client and/or http response.
847864
* If disconnect() has already been called, this does nothing.
848865
*
866+
* @param {string} reason Reason disconnect was called, used for logging.
849867
* @return {Promise} Resolved if disconnect was successful, rejected if errored.
850868
*/
851-
disconnect(error) {
869+
disconnect(reason) {
870+
reason = reason || 'unknown';
852871
// If disconnect has already been called once, do nothing.
872+
// This shouldn't really happen, as the HTTP end listeners will be removed.
853873
if (this.is_finished) {
854-
this.log.debug('KafkaSSE disconnect() has already been called. Doing nothing.');
874+
this.log.debug(`KafkaSSE disconnect() has already been called. Doing nothing in response to: ${reason}`);
855875
return P.resolve();
856876
}
857877
this.is_finished = true;
858878

859-
this.log.info('KafkaSSE disconnecting.');
860-
if (error) {
861-
this._error(error, 'warn');
862-
}
879+
this.log.info(`KafkaSSE disconnecting due to: ${reason}`);
863880

881+
// Remove other HTTP disconnect handlers to prevent disconnect from being fired multiple times.
882+
this.removeHttpEndListeners();
864883

865-
// First deal with the HTTP/SSE response.
866-
// 3 cases:
867-
// - Usually we are disconnecting an active SSEResponse, so just
868-
// end the HTTP response via this.sse.end.
869-
// - If no SSEResponse is active, just end this.res.
870-
// - Else this.res is finished but we still have a reference to it
871-
// so just delete this.res.
872-
return P.resolve().then(() => {
884+
const disconnectHttpPromise = P.resolve().then(() => {
885+
// 3 cases:
886+
// - Usually we are disconnecting an active SSEResponse, so just
887+
// end the HTTP response via this.sse.end.
888+
// - If no SSEResponse is active, just end this.res.
889+
// - Else this.res is finished but we still have a reference to it
890+
// so just delete this.res.
873891
if (this.sse) {
874892
// If we still have SSE, then end the SSEResponse.
875893
// SSEResponse will handle ending thee HTTP Response itself.
876894
const sse = this.sse;
877895
delete this.sse;
878896
delete this.res;
897+
879898
return sse.end()
880-
.catch((e) => this._error(e, 'warn'))
881-
.then(() => {
882-
this.log.debug('KafkaSSE disconnect: Ended SSE + HTTP response.');
883-
});
899+
.then(() => this.log.debug('KafkaSSE disconnect: Ended SSE (HTTP) response.'));
884900
} else if (!this._resFinished()) {
885901
// Else if for disconnect was called and we don't have an SSEResponse,
886902
// (likely because the SSEResponse wasn't ever started), then just
@@ -891,43 +907,48 @@ class KafkaSSE {
891907
delete this.res;
892908
return resolve();
893909
}
910+
894911
delete this.res;
895912
res.on('error', reject);
896913
try {
897-
if (!res.end(resolve)) {
898-
resolve();
899-
}
914+
res.end();
915+
resolve();
900916
} catch(e) {
901917
reject(e);
902918
}
903-
}).catch((e) => this._error(e, 'warn'))
904-
.then(() => {
905-
this.log.debug('KafkaSSE disconnect: Ended HTTP response.');
906-
});
919+
})
920+
.then(() => this.log.debug('KafkaSSE disconnect: Ended HTTP response.'));
907921
} else if (this.res) {
908-
// Else the HTTH Response has already been ended,
922+
// Else the HTTP Response has already been ended,
909923
// so just delete our reference to it.
910924
delete this.res;
911-
this.log.debug('KafkaSSE disconnect: Ended HTTP response.');
925+
this.log.debug('KafkaSSE disconnect: Deleted HTTP response.');
926+
return P.resolve();
912927
}
913-
})
914-
// Now disconnect the Kafka Consumer.
915-
.then(() => {
928+
});
929+
930+
const disconnectKafkaPromise = P.resolve().then(() => {
916931
if (!this.kafkaConsumer) {
932+
this.log.debug("KafkaSSE disconnect: Kafka consumer already deleted, doing nothing.");
917933
return P.resolve();
918934
}
919935
const kafkaConsumer = this.kafkaConsumer;
920936
delete this.kafkaConsumer;
921-
return P.try(() => kafkaConsumer.disconnect())
922-
.catch((e) => this._error(e, 'warn'))
923-
.then(() => {
924-
this.log.debug('KafkaSSE disconnect: Disconnected the Kafka consumer.');
925-
});
937+
938+
return kafkaConsumer.disconnectAsync()
939+
.then(() => this.log.debug("KafkaSSE disconnect: Disconnected the Kafka consumer."));
940+
});
941+
942+
// Emit 'done' when both HTTP response and Kafka are disconnected.
943+
return P.all([disconnectHttpPromise, disconnectKafkaPromise])
944+
.catch((e) => {
945+
this.log.error({err: e}, 'KafkaSSE disconnect: encountered error');
926946
})
927-
.then(() => {
947+
.finally(() => {
928948
this.log.debug('KafkaSSE disconnect: finished.')
929949
return this._eventEmitter.emit('done')
930950
});
951+
931952
}
932953
}
933954

‎lib/SSEResponse.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,7 @@ class SSEResponse {
161161
});
162162
}
163163

164+
164165
_resFinished() {
165166
const res = this.res;
166167
if (!res || res.finished || (res.connection && res.connection.destroyed)) {

0 commit comments

Comments
 (0)