Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

catch potential write errors on fatal flush sync #740

Merged
merged 5 commits into from
Nov 18, 2019
Merged

catch potential write errors on fatal flush sync #740

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d948fbd
Add forceFlushOnFatal flag
Nov 11, 2019
b765a55
Remove unused line
Nov 11, 2019
0fcb023
Do nothing on error
Nov 13, 2019
9920c53
Update lib/levels.js
allevo Nov 15, 2019
c615be0
Remove once
Nov 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/levels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ const levelMethods = {
const stream = this[streamSym]
logFatal.call(this, ...args)
if (typeof stream.flushSync === 'function') {
stream.flushSync()
try {
stream.flushSync()
} catch (e) {
// do nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think we should do something. @mcollina @jsumners what's your thoughts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can't write a log message that the user will maybe see, what do you intend to do to alert them to that message not being written? From the limited understanding I have of the issue, at this point everything is fucked at this point so 🤷‍♂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right but it's worth knowing about? this just swallows it

if we throw an error there will be an understanding that something is wrong

or we can even just console.warn if throwing seems too drastic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the try/catch and it will throw. Wasn't the point of this to remove the throw?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. but the message thrown has clearly caused confusion, we could throw an error with more of an explanation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂ @mcollina

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think swallowing the error is ok here. Essentially we can not write a log line that says we are dying by a fatal error. Notifying a developer is usually done via log files.. which we cannot use in this stance. I think this is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compromise: update the useless “do nothing” comment with a link to the discussion.

allevo marked this conversation as resolved.
Show resolved Hide resolved
}
}
},
error: genLog(levels.error),
Expand Down
14 changes: 14 additions & 0 deletions test/levels.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,17 @@ test('fatal method sync-flushes the destination if sync flushing is available',
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')
mcollina marked this conversation as resolved.
Show resolved Hide resolved
})