pinojs / sonic-boom

Extremely fast utf8 only stream implementation
MIT License
266 stars 41 forks source link

Call flushSync if too much data buffered #185

Open ronag opened 1 year ago

ronag commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Correct.

ronag commented 1 year ago

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 commented 1 year ago

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

mcollina commented 1 year ago

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.