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

Call flushSync if too much data buffered #185

Open
ronag opened this issue Oct 31, 2023 · 6 comments
Open

Call flushSync if too much data buffered #185

ronag opened this issue Oct 31, 2023 · 6 comments

Comments

@ronag
Copy link
Contributor

ronag commented Oct 31, 2023

If the output can't keep up calling async flush will just make things bad and register a lot of drain handlers in ´callFlushCallbackOnDrain´.

If we get to this point we should somehow force some throttling either through some kind of write/flush sync or a spin loop.

@ronag
Copy link
Contributor Author

ronag commented Oct 31, 2023

I'm doing something like this atm:

stream = pino.destination({ sync: false, minLength: 4096 })
let flushing = 0
setInterval(() => {
  if (flushing > 8) {
    stream.flushSync()
  } else {
    flushing++
    stream.flush(() => {
      flushing--
    })
  }
}, flushInterval).unref()

@mcollina
Copy link
Member

This was partially implemented with retryEAGAIN(err, writeBufferLen, remainingBufferLen) which allows to skip a buffer if the target resource is busy and no more can be written to it... which you might not be hitting yet.

Did you implement that workaround becaude you experienced some OOM crash?

@ronag
Copy link
Contributor Author

ronag commented Oct 31, 2023

Did you implement that workaround becaude you experienced some OOM crash?

Correct.

@ronag
Copy link
Contributor Author

ronag commented Oct 31, 2023

I think we might need some more guidance in the docs around async mode and this. Not quite sure what that would be though.

@ronag
Copy link
Contributor Author

ronag commented Oct 31, 2023

Would it make sense to have a "maxLength" in addition to "minLength" which indicates an automatic flush?

@mcollina
Copy link
Member

I would not do an automatic flush because it might cause writing data out of order, it's unlikely but there is a race condition.

If you are not logging on Docker, raise minLength to a few megabytes, and see how it performs.

In your case, I would operate it sync with an even higher minLegth.

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

No branches or pull requests

2 participants