fix: ensure writes are always atomic

[ronag]

Fixes: https://github.com/pinojs/sonic-boom/issues/96

[mcollina]

why did you close this?

[ronag]

missing tests

[ronag]

There is one problem here. We can still write larger than MAX_WRITE if the source doesn't properly pause given the return value of write. But I suppose that's not a big problem? Otherwise the solution becomes more complicated and slower (_buf would need to be an array of strings).

[mcollina]

We can still write larger than MAX_WRITE if the source doesn't properly pause given the return value of write

This is the case of pino.

[ronag]

So this PR is not sufficient.

[ronag]

@mcollina PTAL, doesn't pass tests but I think this could potentially work.

[mcollina]

Agreed, this is quite nice actually

[ronag]

passes all tests except for retry in flushSync on EAGAIN. The whole flushSync I don't understand how it can work when there is a pending async write...

[mcollina]

I'm getting a lot of failures while running your branch locally.

1..12
# failed 8 of 12 tests
# time=2439.583ms

[ronag]

Should be better now. Sorry about that.

[ronag]

the write enormously large buffers test seems to take a very long time to finish now...

[mcollina]

I can see some slowdown but it seems acceptable to solve this problem:

This branch:

benchSonic*1000: 521.245ms
benchSonicSync*1000: 6.851s
benchSonic4k*1000: 466.171ms
benchSonicSync4k*1000: 336.917ms
benchCore*1000: 4.511s
benchConsole*1000: 9.480s

master:

benchSonic*1000: 498.087ms
benchSonicSync*1000: 6.710s
benchSonic4k*1000: 431.978ms
benchSonicSync4k*1000: 295.552ms
benchCore*1000: 4.516s
benchConsole*1000: 9.521s

We can see if there are a few areas were we could optimize.

[mcollina]

The following fixes the broken test:

diff --git a/index.js b/index.js
index fc19cdf..a3118a6 100644
--- a/index.js
+++ b/index.js
@@ -145,8 +145,8 @@ function SonicBoom (opts) {
         this.emit('error', err)
       }
       return
-    }
-
+    }
+
     this._len -= n
     this._writingBuf = this._writingBuf.slice(n)

@@ -335,12 +335,14 @@ SonicBoom.prototype.flushSync = function () {
   }

   while (this._bufs.length) {
+    const buf = this._bufs.shift()
     try {
-      this._len -= fs.writeSync(this.fd, this._bufs.shift(), 'utf8')
+      this._len -= fs.writeSync(this.fd, buf, 'utf8')
     } catch (err) {
       if (err.code !== 'EAGAIN') {
         throw err
       }
+      this._bufs.unshift(buf)

       sleep(BUSY_WRITE_TIMEOUT)
     }

Essentially the flushSync() implementation did not reinsert the shifted buffer in.

[ronag]

Thanks! Fixed.

[ronag]

https://github.com/pinojs/sonic-boom/pull/102