Skip to content

fix(multistream): isolate per-stream write() errors so remaining streams still receive log data - #2455

Open
LeSingh1 wants to merge 2 commits into
pinojs:mainfrom
LeSingh1:fix/multistream-write-error-isolation
Open

fix(multistream): isolate per-stream write() errors so remaining streams still receive log data#2455
LeSingh1 wants to merge 2 commits into
pinojs:mainfrom
LeSingh1:fix/multistream-write-error-isolation

Conversation

@LeSingh1

Copy link
Copy Markdown

Problem

pino.multistream().write() iterates over all registered streams and calls stream.write(data) on each. There is no error isolation around individual writes: if any stream's synchronous write() throws, the iteration loop aborts immediately and every stream that comes after the failing one in the sorted list never receives the log line. Logs are silently dropped with no indication that anything went wrong.

Minimal reproduction:

const pino = require('pino')

let received = 0
const goodStream = { write () { received++ } }
const badStream  = { write () { throw new Error('boom') } }

const multi = pino.multistream([
  { level: 'info', stream: badStream  },   // sorted first (same level)
  { level: 'info', stream: goodStream }
])

const log = pino({ level: 'info' }, multi)
log.info('hello')

console.log(received) // prints 0 — goodStream never received the line!

Fix

Wrap each stream.write(data) call in a try/catch inside multistream.write(). The caught error is surfaced via a dedicated EventEmitter (errorEmitter) so callers can observe it with res.on('error', fn) — without interfering with the existing res.emit() method, which propagates events like 'message' down to every child stream for pino's internal config protocol.

When no 'error' listener is registered the error is swallowed, preserving the existing non-throwing contract of multistream.write().

What changed in lib/multistream.js:

  • Added an internal errorEmitter (EventEmitter) instance.
  • Exposed on / once / removeListener / off on res delegating to errorEmitter.
  • Wrapped stream.write(data) in try/catch; emits 'error' on errorEmitter when there is at least one listener, otherwise swallows.

New tests (in test/multistream.test.js)

  1. multistream.write continues writing to remaining streams when one stream throws synchronously — asserts that a stream ordered after a throwing stream still receives all log lines and that an 'error' event is emitted for each throwing write.
  2. multistream.write does not throw when a stream write() throws and no error listener is registered — asserts doesNotThrow when no listener is attached.

Test results

All 32 multistream tests pass (including the 2 new ones), and the full basic + serializer suites (126 tests) continue to pass.

Checklist

  • Existing tests pass
  • New regression tests added
  • No change to the res.emit() stream-propagation behaviour
  • Backward compatible (callers that do not attach a res.on('error') listener see no behaviour change)

…logs

Previously, if any stream's synchronous write() threw an exception inside
multistream.write(), the loop would abort immediately and all streams ordered
after the failing one would never receive the log line.

This fix wraps each stream.write(data) call in a try/catch. The caught error
is surfaced via a dedicated errorEmitter so callers can observe it with
res.on('error', fn) without interfering with the existing res.emit() behaviour
(which propagates events like 'message' down to every child stream). When no
error listener is registered the error is swallowed to preserve the
non-throwing contract of multistream.write().

Two new tests are added:
- one that asserts subsequent streams receive all log lines when an earlier
  stream throws synchronously
- one that asserts no exception escapes when there is no error listener

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the best outcome would be to move the res object to
be a full EventEmitter instead.

@LeSingh1
LeSingh1 force-pushed the fix/multistream-write-error-isolation branch from b60e66e to b486f9a Compare July 10, 2026 14:42
Comment thread lib/multistream.js
stream.emit(...args)
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahum, this doesn't look a good idea after all. Do we need the return value to have an emit property?

If we can't remove this, the previous approach was in fact better. My bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants