Skip to content

fix: invoke flush callback when transport worker is unref'd - #2482

Open
Hashim1999164 wants to merge 1 commit into
pinojs:mainfrom
Hashim1999164:fix/flush-callback-idle-loop
Open

fix: invoke flush callback when transport worker is unref'd#2482
Hashim1999164 wants to merge 1 commit into
pinojs:mainfrom
Hashim1999164:fix/flush-callback-idle-loop

Conversation

@Hashim1999164

Copy link
Copy Markdown

Summary

  • Transport workers are unref'd after ready so the process can exit, which meant logger.flush(cb) never got its completion message on an idle event loop.
  • Keep the worker referenced while a flush callback is in flight, and skip the ready-time unref if a flush is already pending.
  • Add an idle-loop regression fixture and drop the old test-side keep-alive workaround.

Fixes #2481

Test plan

  • npx borp --timeout 60000 test/transport/sync-false.test.js test/syncfalse.test.js test/exit.test.js test/transport/core.test.js (51 passed)
  • node test/fixtures/transport-flush-idle.js prints callback-fired and exits 0

@aquie00t

Copy link
Copy Markdown

Thanks for jumping on this — the ref-during-flush approach is exactly right!

One design question for the maintainers before this lands: the root cause
actually lives in thread-stream, where requestWorkerFlush registers the
callback in flushCallbacks and posts the FLUSH message, but the worker is
already unref'd, so an idle event loop drains before the completion message
arrives.

Fixing it there — ref the worker while any callback is in flushCallbacks,
unref once the map empties — would:

  • keep pino's lib/proto.js unchanged (no new state for tracking pending flushes)
  • automatically fix the same behavior for other thread-stream consumers
    (pino-elasticsearch, custom transports, etc.)
  • respect the encapsulation: pino calls stream.flush(cb) and trusts
    the stream to honor the callback contract

Not a blocker for this PR — just wanted to surface the alternative in case
the deeper fix feels cleaner. Happy to help either way.

@Hashim1999164

Copy link
Copy Markdown
Author

Thanks for the thread-stream perspective. Happy to keep this PR as the pino-side workaround if maintainers prefer shipping the fix here first, or reshape it if the preferred path is fixing ref/unref in thread-stream instead. Looking for maintainer guidance either way.

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.

logger.flush(cb) never invokes the callback on an idle event loop

2 participants