From d948fbd323d76b3f3ccdaa77cabe09bc610b390d Mon Sep 17 00:00:00 2001 From: Tommaso Allevi Date: Mon, 11 Nov 2019 11:51:59 +0100 Subject: [PATCH 1/5] Add forceFlushOnFatal flag --- lib/levels.js | 5 +++-- lib/symbols.js | 5 ++++- pino.js | 12 ++++++++---- test/levels.test.js | 18 +++++++++++++++++- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/lib/levels.js b/lib/levels.js index a9e0e555f..9ede9f11c 100644 --- a/lib/levels.js +++ b/lib/levels.js @@ -6,7 +6,8 @@ const { useLevelLabelsSym, changeLevelNameSym, useOnlyCustomLevelsSym, - streamSym + streamSym, + forceFlushOnFatalSym } = require('./symbols') const { noop, genLog } = require('./tools') @@ -23,7 +24,7 @@ const levelMethods = { fatal (...args) { const stream = this[streamSym] logFatal.call(this, ...args) - if (typeof stream.flushSync === 'function') { + if (this[forceFlushOnFatalSym] && typeof stream.flushSync === 'function') { stream.flushSync() } }, diff --git a/lib/symbols.js b/lib/symbols.js index 38c3609e6..810f0d12e 100644 --- a/lib/symbols.js +++ b/lib/symbols.js @@ -26,6 +26,8 @@ const messageKeySym = Symbol('pino.messageKey') const wildcardFirstSym = Symbol('pino.wildcardFirst') +const forceFlushOnFatalSym = Symbol('pino.forceFlushOnFatal') + // public symbols, no need to use the same pino // version for these const serializersSym = Symbol.for('pino.serializers') @@ -56,5 +58,6 @@ module.exports = { changeLevelNameSym, wildcardGsym, needsMetadataGsym, - useOnlyCustomLevelsSym + useOnlyCustomLevelsSym, + forceFlushOnFatalSym } diff --git a/pino.js b/pino.js index 14ad86c8c..a77995eb1 100644 --- a/pino.js +++ b/pino.js @@ -29,7 +29,8 @@ const { messageKeySym, useLevelLabelsSym, changeLevelNameSym, - useOnlyCustomLevelsSym + useOnlyCustomLevelsSym, + forceFlushOnFatalSym } = symbols const { epochTime, nullTime } = time const { pid } = process @@ -50,7 +51,8 @@ const defaultOptions = { redact: null, customLevels: null, changeLevelName: 'level', - useOnlyCustomLevels: false + useOnlyCustomLevels: false, + forceFlushOnFatal: false } const normalize = createArgsNormalizer(defaultOptions) @@ -71,7 +73,8 @@ function pino (...args) { customLevels, useLevelLabels, changeLevelName, - useOnlyCustomLevels + useOnlyCustomLevels, + forceFlushOnFatal } = opts const stringifiers = redact ? redaction(redact, stringify) : {} @@ -110,7 +113,8 @@ function pino (...args) { [formatOptsSym]: formatOpts, [messageKeySym]: messageKey, [serializersSym]: serializers, - [chindingsSym]: chindings + [chindingsSym]: chindings, + [forceFlushOnFatalSym]: forceFlushOnFatal } Object.setPrototypeOf(instance, proto) diff --git a/test/levels.test.js b/test/levels.test.js index 4643ad853..484391449 100644 --- a/test/levels.test.js +++ b/test/levels.test.js @@ -409,7 +409,23 @@ test('fatal method sync-flushes the destination if sync flushing is available', stream.flushSync = () => { pass('destination flushed') } - const instance = pino(stream) + const instance = pino({ forceFlushOnFatal: true }, stream) + instance.fatal('this is fatal') + await once(stream, 'data') + doesNotThrow(() => { + stream.flushSync = undefined + instance.fatal('this is fatal') + }) +}) + +test('fatal method should not sync-flushes the destination if sync flushing is disavailable', async ({ pass, fail, doesNotThrow, plan }) => { + plan(1) + const stream = sink() + stream.flushSync = () => fail('flushSync should not be called') + stream.flush = () => { + pass('destination flushed') + } + const instance = pino({ forceFlushOnFatal: false }, stream) instance.fatal('this is fatal') await once(stream, 'data') doesNotThrow(() => { From b765a5541a0781e8e354590875c0e4edddb04d37 Mon Sep 17 00:00:00 2001 From: Tommaso Allevi Date: Mon, 11 Nov 2019 11:56:31 +0100 Subject: [PATCH 2/5] Remove unused line --- test/levels.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/levels.test.js b/test/levels.test.js index 484391449..66b312c20 100644 --- a/test/levels.test.js +++ b/test/levels.test.js @@ -429,7 +429,6 @@ test('fatal method should not sync-flushes the destination if sync flushing is d instance.fatal('this is fatal') await once(stream, 'data') doesNotThrow(() => { - stream.flushSync = undefined instance.fatal('this is fatal') }) }) From 0fcb023abd78b39463809ae88f0789cc76c8ad54 Mon Sep 17 00:00:00 2001 From: Tommaso Allevi Date: Wed, 13 Nov 2019 22:08:48 +0100 Subject: [PATCH 3/5] Do nothing on error --- lib/levels.js | 11 +++++++---- lib/symbols.js | 5 +---- pino.js | 12 ++++-------- test/levels.test.js | 27 +++++++++++++-------------- 4 files changed, 25 insertions(+), 30 deletions(-) diff --git a/lib/levels.js b/lib/levels.js index 9ede9f11c..995b54e0e 100644 --- a/lib/levels.js +++ b/lib/levels.js @@ -6,8 +6,7 @@ const { useLevelLabelsSym, changeLevelNameSym, useOnlyCustomLevelsSym, - streamSym, - forceFlushOnFatalSym + streamSym } = require('./symbols') const { noop, genLog } = require('./tools') @@ -24,8 +23,12 @@ const levelMethods = { fatal (...args) { const stream = this[streamSym] logFatal.call(this, ...args) - if (this[forceFlushOnFatalSym] && typeof stream.flushSync === 'function') { - stream.flushSync() + if (typeof stream.flushSync === 'function') { + try { + stream.flushSync() + } catch (e) { + // do nothing + } } }, error: genLog(levels.error), diff --git a/lib/symbols.js b/lib/symbols.js index 810f0d12e..38c3609e6 100644 --- a/lib/symbols.js +++ b/lib/symbols.js @@ -26,8 +26,6 @@ const messageKeySym = Symbol('pino.messageKey') const wildcardFirstSym = Symbol('pino.wildcardFirst') -const forceFlushOnFatalSym = Symbol('pino.forceFlushOnFatal') - // public symbols, no need to use the same pino // version for these const serializersSym = Symbol.for('pino.serializers') @@ -58,6 +56,5 @@ module.exports = { changeLevelNameSym, wildcardGsym, needsMetadataGsym, - useOnlyCustomLevelsSym, - forceFlushOnFatalSym + useOnlyCustomLevelsSym } diff --git a/pino.js b/pino.js index a77995eb1..14ad86c8c 100644 --- a/pino.js +++ b/pino.js @@ -29,8 +29,7 @@ const { messageKeySym, useLevelLabelsSym, changeLevelNameSym, - useOnlyCustomLevelsSym, - forceFlushOnFatalSym + useOnlyCustomLevelsSym } = symbols const { epochTime, nullTime } = time const { pid } = process @@ -51,8 +50,7 @@ const defaultOptions = { redact: null, customLevels: null, changeLevelName: 'level', - useOnlyCustomLevels: false, - forceFlushOnFatal: false + useOnlyCustomLevels: false } const normalize = createArgsNormalizer(defaultOptions) @@ -73,8 +71,7 @@ function pino (...args) { customLevels, useLevelLabels, changeLevelName, - useOnlyCustomLevels, - forceFlushOnFatal + useOnlyCustomLevels } = opts const stringifiers = redact ? redaction(redact, stringify) : {} @@ -113,8 +110,7 @@ function pino (...args) { [formatOptsSym]: formatOpts, [messageKeySym]: messageKey, [serializersSym]: serializers, - [chindingsSym]: chindings, - [forceFlushOnFatalSym]: forceFlushOnFatal + [chindingsSym]: chindings } Object.setPrototypeOf(instance, proto) diff --git a/test/levels.test.js b/test/levels.test.js index 66b312c20..461937827 100644 --- a/test/levels.test.js +++ b/test/levels.test.js @@ -409,7 +409,7 @@ test('fatal method sync-flushes the destination if sync flushing is available', stream.flushSync = () => { pass('destination flushed') } - const instance = pino({ forceFlushOnFatal: true }, stream) + const instance = pino(stream) instance.fatal('this is fatal') await once(stream, 'data') doesNotThrow(() => { @@ -418,17 +418,16 @@ test('fatal method sync-flushes the destination if sync flushing is available', }) }) -test('fatal method should not sync-flushes the destination if sync flushing is disavailable', async ({ pass, fail, doesNotThrow, plan }) => { - plan(1) - const stream = sink() - stream.flushSync = () => fail('flushSync should not be called') - stream.flush = () => { - pass('destination flushed') - } - const instance = pino({ forceFlushOnFatal: false }, stream) - instance.fatal('this is fatal') - await once(stream, 'data') - doesNotThrow(() => { - instance.fatal('this is fatal') - }) +test('fatal method should call async when sync-flushing fails', ({ equal, fail, doesNotThrow, plan }) => { + plan(2) + const messages = [ + 'this is fatal 1' + ] + const stream = sink((result) => equal(result.msg, messages.shift())) + stream.flushSync = () => { throw new Error('Error') } + stream.flush = () => fail('flush should be called') + + const instance = pino(stream) + doesNotThrow(() => instance.fatal(messages[0])) + once(stream, 'data') }) From 9920c539e4acb3d0e1067908238c8e8cab94f916 Mon Sep 17 00:00:00 2001 From: Tommaso Allevi Date: Fri, 15 Nov 2019 20:43:12 +0100 Subject: [PATCH 4/5] Update lib/levels.js Co-Authored-By: David Mark Clements --- lib/levels.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/levels.js b/lib/levels.js index 995b54e0e..c4ebbdce2 100644 --- a/lib/levels.js +++ b/lib/levels.js @@ -27,7 +27,7 @@ const levelMethods = { try { stream.flushSync() } catch (e) { - // do nothing + // https://github.com/pinojs/pino/pull/740#discussion_r346788313 } } }, From c615be0a9f1eb36508f5598cddb8d06c4b5237a7 Mon Sep 17 00:00:00 2001 From: Tommaso Allevi Date: Sun, 17 Nov 2019 14:41:14 +0100 Subject: [PATCH 5/5] Remove once --- test/levels.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/levels.test.js b/test/levels.test.js index 461937827..b7ac1cc80 100644 --- a/test/levels.test.js +++ b/test/levels.test.js @@ -429,5 +429,4 @@ test('fatal method should call async when sync-flushing fails', ({ equal, fail, const instance = pino(stream) doesNotThrow(() => instance.fatal(messages[0])) - once(stream, 'data') })