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.

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

Coerce string integer destinations to file descriptors #1180

Merged
merged 2 commits into from Nov 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions docs/api.md
Expand Up @@ -422,7 +422,7 @@ Default: `pino.destination(1)` (STDOUT)
The `destination` parameter, at a minimum must be an object with a `write` method.
An ordinary Node.js `stream` can be passed as the destination (such as the result
of `fs.createWriteStream`) but for peak log writing performance it is strongly
recommended to use `pino.destination` to create the destination stream.
recommended to use `pino.destination` to create the destination stream.
Note that the `destination` parameter can be the result of `pino.transport()`.

```js
Expand All @@ -449,6 +449,10 @@ However, there are some special instances where `pino.destination` is not used a

In these cases `process.stdout` is used instead.

Note: If the parameter is a string integer, e.g. `'1'`, it will be coerced to
a number and used as a file descriptor. If this is not desired, provide a full
path, e.g. `/tmp/1`.

* See [`pino.destination`](#pino-destination)

<a id="metadata"></a>
Expand Down Expand Up @@ -1010,7 +1014,7 @@ is flushed and all data synced before the process exits.
Note that calling `process.exit()` on the main thread will stop the event loop on the main thread from turning. As a result,
using `console.log` and `process.stdout` after the main thread called `process.exit()` will not produce any output.

If you are embedding/integrating pino within your framework, you will need to make pino aware of the script that is calling it,
If you are embedding/integrating pino within your framework, you will need to make pino aware of the script that is calling it,
like so:

```js
Expand Down
2 changes: 1 addition & 1 deletion docs/transports.md
Expand Up @@ -262,7 +262,7 @@ const transport = pino.transport({
pino(transport)
```

The `options.destination` property may also be a number to represent a file descriptor. Typically this would be `1` to write to STDOUT or `2` to write to STDERR. If `options.destination` is not set, it defaults to `1` which means logs will be written to STDOUT.
The `options.destination` property may also be a number to represent a filedescriptor. Typically this would be `1` to write to STDOUT or `2` to write to STDERR. If `options.destination` is not set, it defaults to `1` which means logs will be written to STDOUT. If `options.destination` is a string integer, e.g. `'1'`, it will be coerced to a number and used as a file descriptor. If this is not desired, provide a full path, e.g. `/tmp/1`.
Copy link
Member

Choose a reason for hiding this comment

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

The docs for pino.destination should be updated too.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ will do. I literally went back to bed after this.


The difference between using the `pino/file` transport builtin and using `pino.destination` is that `pino.destination` runs in the main thread, whereas `pino/file` sets up `pino.destination` in a worker thread.

Expand Down
19 changes: 18 additions & 1 deletion lib/tools.js
Expand Up @@ -520,6 +520,22 @@ function setMetadataProps (dest, that) {
}
}

/**
* Convert a string integer file descriptor to a proper native integer
* file descriptor.
*
* @param {string} destination The file descriptor string to attempt to convert.
*
* @returns {Number}
*/
function normalizeDestFileDescriptor (destination) {
const fd = Number(destination)
if (typeof destination === 'string' && Number.isFinite(fd)) {
return fd
}
return destination
}

module.exports = {
noop,
buildSafeSonicBoom,
Expand All @@ -530,5 +546,6 @@ module.exports = {
createArgsNormalizer,
final,
stringify,
buildFormatters
buildFormatters,
normalizeDestFileDescriptor
}
9 changes: 5 additions & 4 deletions pino.js
Expand Up @@ -15,8 +15,9 @@ const {
final,
buildSafeSonicBoom,
buildFormatters,
noop,
stringify
stringify,
normalizeDestFileDescriptor,
noop
} = require('./lib/tools')
const { version } = require('./lib/meta')
const {
Expand Down Expand Up @@ -191,10 +192,10 @@ module.exports = pino

module.exports.destination = (dest = process.stdout.fd) => {
if (typeof dest === 'object') {
dest.dest = dest.dest || process.stdout.fd
dest.dest = normalizeDestFileDescriptor(dest.dest || process.stdout.fd)
return buildSafeSonicBoom(dest)
} else {
return buildSafeSonicBoom({ dest, minLength: 0, sync: true })
return buildSafeSonicBoom({ dest: normalizeDestFileDescriptor(dest), minLength: 0, sync: true })
}
}

Expand Down
9 changes: 9 additions & 0 deletions test/fixtures/transport-string-stdout.js
@@ -0,0 +1,9 @@
'use strict'

const pino = require('../..')
const transport = pino.transport({
target: 'pino/file',
options: { destination: '1' }
})
const logger = pino(transport)
logger.info('Hello')
12 changes: 12 additions & 0 deletions test/transport/core.test.js
Expand Up @@ -377,6 +377,18 @@ test('log and exit before ready', async ({ not }) => {
not(strip(actual).match(/Hello/), null)
})

test('string integer destination', async ({ not }) => {
let actual = ''
const child = execa(process.argv[0], [join(__dirname, '..', 'fixtures', 'transport-string-stdout.js')])

child.stdout.pipe(writer((s, enc, cb) => {
actual += s
cb()
}))
await once(child, 'close')
not(strip(actual).match(/Hello/), null)
})

test('pino transport options with target', async ({ teardown, same }) => {
const destination = join(
os.tmpdir(),
Expand Down