Skip to content

fs: port SonicBoom module to fs module as FastUtf8Stream #58897

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 30, 2025

As a first step to porting portions of the pino structured logger into the runtime, this commit ports the SonicBoom module to the fs module as FastUtf8Stream. Sonicboom is a dependency of pino.

This is a faithful port of the SonicBoom module with some modern updates, such as converting to a Class and using Symbol.dispose. The bulk of the implementation is unchanged from the original.

Refs: #49296 (comment)

/cc @mcollina @ronag @kibertoad @jsumners @mmarchini @feugy

@jasnell jasnell requested a review from mcollina June 30, 2025 02:49
@jasnell jasnell marked this pull request as draft June 30, 2025 02:49
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg
  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. tools Issues and PRs related to the tools directory. labels Jun 30, 2025
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 30, 2025
Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 74.86457% with 232 lines in your changes missing coverage. Please review.

Project coverage is 89.99%. Comparing base (049664b) to head (40b4795).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/streams/fast-utf8-stream.js 74.58% 231 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58897      +/-   ##
==========================================
- Coverage   90.06%   89.99%   -0.08%     
==========================================
  Files         645      646       +1     
  Lines      189130   190053     +923     
  Branches    37094    37272     +178     
==========================================
+ Hits       170339   171036     +697     
- Misses      11511    11730     +219     
- Partials     7280     7287       +7     
Files with missing lines Coverage Δ
lib/fs.js 98.18% <100.00%> (+<0.01%) ⬆️
lib/internal/streams/fast-utf8-stream.js 74.58% <74.58%> (ø)

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

huge +1. This would be beneficial for other loggers too.

@jasnell jasnell force-pushed the jasnell/port-sonicboom branch from 41f2c92 to 5631e21 Compare June 30, 2025 13:12
@jasnell
Copy link
Member Author

jasnell commented Jun 30, 2025

Tests updated. @mcollina ... So far I have not ported any of the tests that require proxyquire. I'm not sure what's the best way to port that behavior yet. I think, however, that I'd like to get this landed as experimental first and then incrementally improve the test coverage.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Some of the worst bugs we fixed are being tests by the proxyquire-based tests (those are error conditions that are impossible to replicate without mocking). They are the battle scars, and they are important to keep around / being ported.

I'm ok for landing this as-is, but can you create a tracking issue for all the tests that you skipped so that they can be ported around?

@jasnell
Copy link
Member Author

jasnell commented Jun 30, 2025

Absolutely. It's really all of the tests that use proxyquire. We can port those by monkey patching the fs APIs. Not as clean but doable. Will just take time

@jasnell jasnell force-pushed the jasnell/port-sonicboom branch 3 times, most recently from bb11c97 to e03efc9 Compare July 4, 2025 16:37
@jasnell jasnell marked this pull request as ready for review July 4, 2025 16:37
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnell jasnell force-pushed the jasnell/port-sonicboom branch from e03efc9 to da471d7 Compare July 4, 2025 18:25
@jasnell jasnell force-pushed the jasnell/port-sonicboom branch from da471d7 to f5136bd Compare July 5, 2025 19:40
@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell force-pushed the jasnell/port-sonicboom branch from f5136bd to 583dcfb Compare July 5, 2025 23:01
@jasnell
Copy link
Member Author

jasnell commented Jul 5, 2025

@mertcanaltin ... looks like the tests here are hanging and failing consistently. Looks like switching away from using node:test and restructuring the tests helps clear it. Will need to work through refactoring a bit more.

@jasnell jasnell marked this pull request as draft July 5, 2025 23:02
@mertcanaltin
Copy link
Member

mertcanaltin commented Jul 6, 2025

@mertcanaltin ... looks like the tests here are hanging and failing consistently. Looks like switching away from using node:test and restructuring the tests helps clear it. Will need to work through refactoring a bit more.

I would love to help you with this, I'll try an arrangement

@mertcanaltin
Copy link
Member

mertcanaltin commented Jul 7, 2025

@jasnell Hi i sent this branch a commit that implements the node:test avoidance state you mentioned
7ac821d

@H4ad
Copy link
Member

H4ad commented Jul 9, 2025

Here's a little patch to implements the tests that uses proxyquire:

diff.patch
diff --git a/lib/fs.js b/lib/fs.js
index f8c78625b7b..f9a541fa78e 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -167,7 +167,7 @@ let FileWriteStream;
 let FastUtf8Stream;
 
 function lazyLoadFastUtf8Stream() {
-  FastUtf8Stream ??= require('internal/streams/fast-utf8-stream');
+  FastUtf8Stream ??= require('internal/streams/fast-utf8-stream').FastUtf8Stream;
 }
 
 // Ensure that callbacks run in the global context. Only use this function
diff --git a/lib/internal/streams/fast-utf8-stream.js b/lib/internal/streams/fast-utf8-stream.js
index cd393605c7a..8ac4082034f 100644
--- a/lib/internal/streams/fast-utf8-stream.js
+++ b/lib/internal/streams/fast-utf8-stream.js
@@ -66,6 +66,8 @@ function sleep(ms) {
   AtomicsWait(kNil, 0, 0, Number(ms));
 }
 
+const kCustomFs = Symbol('customFs');
+
 // 16 KB. Don't write more than docker buffer size.
 // https://github.com/moby/moby/blob/513ec73831269947d38a644c278ce3cac36783b2/daemon/logger/copier.go#L13
 const kMaxWrite = 16 * 1024;
@@ -108,6 +110,7 @@ class FastUtf8Stream extends EventEmitter {
   #actualWrite;
   #fsWriteSync;
   #fsWrite;
+  #fs;
 
   constructor(options = kNullPrototype) {
     validateObject(options, 'options');
@@ -144,6 +147,7 @@ class FastUtf8Stream extends EventEmitter {
     this.#mode = mode;
     this.#retryEAGAIN = retryEAGAIN || (() => true);
     this.#mkdir = mkdir || false;
+    this.#fs = options[kCustomFs] || fs;
 
     validateUint32(this.#hwm, 'options.hwm');
     validateUint32(this.#minLength, 'options.minLength');
@@ -163,16 +167,16 @@ class FastUtf8Stream extends EventEmitter {
       this.#flush = (...args) => this.#flushBuffer(...args);
       this.#flushSync = (...args) => this.#flushBufferSync(...args);
       this.#actualWrite = (...args) => this.#actualWriteBuffer(...args);
-      this.#fsWriteSync = () => fs.writeSync(this.#fd, this.#writingBuf);
-      this.#fsWrite = () => fs.write(this.#fd, this.#writingBuf, (...args) => this.#release(...args));
+      this.#fsWriteSync = () => this.#fs.writeSync(this.#fd, this.#writingBuf);
+      this.#fsWrite = () => this.#fs.write(this.#fd, this.#writingBuf, (...args) => this.#release(...args));
     } else {
       this.#writingBuf = '';
       this.#write = (...args) => this.#writeUtf8(...args);
       this.#flush = (...args) => this.#flushUtf8(...args);
       this.#flushSync = (...args) => this.#flushSyncUtf8(...args);
       this.#actualWrite = (...args) => this.#actualWriteUtf8(...args);
-      this.#fsWriteSync = () => fs.writeSync(this.#fd, this.#writingBuf, 'utf8');
-      this.#fsWrite = () => fs.write(this.#fd, this.#writingBuf, 'utf8', (...args) => this.#release(...args));
+      this.#fsWriteSync = () => this.#fs.writeSync(this.#fd, this.#writingBuf, 'utf8');
+      this.#fsWrite = () => this.#fs.write(this.#fd, this.#writingBuf, 'utf8', (...args) => this.#release(...args));
     }
 
     if (typeof fd === 'number') {
@@ -239,7 +243,7 @@ class FastUtf8Stream extends EventEmitter {
     const fd = this.#fd;
     this.once('ready', () => {
       if (fd !== this.#fd) {
-        fs.close(fd, (err) => {
+        this.#fs.close(fd, (err) => {
           if (err) {
             return this.emit('error', err);
           }
@@ -355,7 +359,7 @@ class FastUtf8Stream extends EventEmitter {
     }
 
     if (this.#fsync) {
-      fs.fsyncSync(this.#fd);
+      this.#fs.fsyncSync(this.#fd);
     }
 
     const len = this.#len;
@@ -442,20 +446,20 @@ class FastUtf8Stream extends EventEmitter {
 
     if (this.#sync) {
       try {
-        if (this.#mkdir) fs.mkdirSync(path.dirname(file), { recursive: true });
-        const fd = fs.openSync(file, flags, mode);
+        if (this.#mkdir) this.#fs.mkdirSync(path.dirname(file), { recursive: true });
+        const fd = this.#fs.openSync(file, flags, mode);
         fileOpened(null, fd);
       } catch (err) {
         fileOpened(err);
         throw err;
       }
     } else if (this.#mkdir) {
-      fs.mkdir(path.dirname(file), { recursive: true }, (err) => {
+      this.#fs.mkdir(path.dirname(file), { recursive: true }, (err) => {
         if (err) return fileOpened(err);
-        fs.open(file, flags, mode, fileOpened);
+        this.#fs.open(file, flags, mode, fileOpened);
       });
     } else {
-      fs.open(file, flags, mode, fileOpened);
+      this.#fs.open(file, flags, mode, fileOpened);
     }
   }
 
@@ -497,14 +501,14 @@ class FastUtf8Stream extends EventEmitter {
     const closeWrapped = () => {
       // We skip errors in fsync
       if (this.#fd !== 1 && this.#fd !== 2) {
-        fs.close(this.#fd, done);
+        this.#fs.close(this.#fd, done);
       } else {
         done();
       }
     };
 
     try {
-      fs.fsync(this.#fd, closeWrapped);
+      this.#fs.fsync(this.#fd, closeWrapped);
     } catch {
       // Intentionally empty.
     }
@@ -516,7 +520,7 @@ class FastUtf8Stream extends EventEmitter {
 
     if (this.#sync) {
       try {
-        const written = fs.writeSync(this.#fd, this.#writingBuf);
+        const written = this.#fs.writeSync(this.#fd, this.#writingBuf);
         this.#release(null, written);
       } catch (err) {
         this.#release(err);
@@ -526,7 +530,7 @@ class FastUtf8Stream extends EventEmitter {
       // we do it here to avoid the overhead of calculating the buffer size
       // in releaseWritingBuf.
       this.#writingBuf = Buffer.from(this.#writingBuf);
-      fs.write(this.#fd, this.#writingBuf, (...args) => this.#release(...args));
+      this.#fs.write(this.#fd, this.#writingBuf, (...args) => this.#release(...args));
     }
   }
 
@@ -536,13 +540,13 @@ class FastUtf8Stream extends EventEmitter {
 
     if (this.#sync) {
       try {
-        const written = fs.writeSync(this.#fd, this.#writingBuf, 'utf8');
+        const written = this.#fs.writeSync(this.#fd, this.#writingBuf, 'utf8');
         this.#release(null, written);
       } catch (err) {
         this.#release(err);
       }
     } else {
-      fs.write(this.#fd, this.#writingBuf, 'utf8', (...args) => this.#release(...args));
+      this.#fs.write(this.#fd, this.#writingBuf, 'utf8', (...args) => this.#release(...args));
     }
   }
 
@@ -566,7 +570,7 @@ class FastUtf8Stream extends EventEmitter {
         buf = mergeBuf(this.#bufs[0], this.#lens[0]);
       }
       try {
-        const n = fs.writeSync(this.#fd, buf);
+        const n = this.#fs.writeSync(this.#fd, buf);
         buf = buf.subarray(n);
         this.#len = MathMax(this.#len - n, 0);
         if (buf.length <= 0) {
@@ -604,7 +608,7 @@ class FastUtf8Stream extends EventEmitter {
         buf = this.#bufs[0];
       }
       try {
-        const n = fs.writeSync(this.#fd, buf, 'utf8');
+        const n = this.#fs.writeSync(this.#fd, buf, 'utf8');
         const releasedBufObj = releaseWritingBuf(buf, this.#len, n);
         buf = releasedBufObj.writingBuf;
         this.#len = releasedBufObj.len;
@@ -622,7 +626,7 @@ class FastUtf8Stream extends EventEmitter {
     }
 
     try {
-      fs.fsyncSync(this.#fd);
+      this.#fs.fsyncSync(this.#fd);
     } catch {
       // Skip the error. The fd might not support fsync.
     }
@@ -634,7 +638,7 @@ class FastUtf8Stream extends EventEmitter {
       // Only if _fsync is false to avoid double fsync
       if (!this.#fsync) {
         try {
-          fs.fsync(this.#fd, (err) => {
+          this.#fs.fsync(this.#fd, (err) => {
             this.#flushPending = false;
             cb(err);
           });
@@ -821,4 +825,7 @@ function mergeBuf(bufs, len) {
   return Buffer.concat(bufs, len);
 }
 
-module.exports = FastUtf8Stream;
+module.exports = {
+  FastUtf8Stream,
+  kCustomFs,
+};
diff --git a/test/parallel/test-fastutf8stream-destroy.js b/test/parallel/test-fastutf8stream-destroy.js
index 674d040d023..9eaea55bf66 100644
--- a/test/parallel/test-fastutf8stream-destroy.js
+++ b/test/parallel/test-fastutf8stream-destroy.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-end.js b/test/parallel/test-fastutf8stream-end.js
index f66a0107388..6587564e567 100644
--- a/test/parallel/test-fastutf8stream-end.js
+++ b/test/parallel/test-fastutf8stream-end.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-flush-mocks.js b/test/parallel/test-fastutf8stream-flush-mocks.js
index 273e094e08a..dc8cc42b7ae 100644
--- a/test/parallel/test-fastutf8stream-flush-mocks.js
+++ b/test/parallel/test-fastutf8stream-flush-mocks.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-flush-sync.js b/test/parallel/test-fastutf8stream-flush-sync.js
index eab7e89060b..2f1de012146 100644
--- a/test/parallel/test-fastutf8stream-flush-sync.js
+++ b/test/parallel/test-fastutf8stream-flush-sync.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream, kCustomFs } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
@@ -64,3 +64,113 @@ it('flushSync', async (t) => {
   }));
   await promise;
 });
+
+it('retry in flushSync on EAGAIN', async (t) => {
+  const dest = file()
+  const fd = fs.openSync(dest, 'w')
+  let shouldThrow = false;
+  const stream = new FastUtf8Stream({
+    fd,
+    sync: false,
+    minLength: 0,
+    [kCustomFs]: {
+      ...fs,
+      writeSync: common.mustCallAtLeast((fd, buf, enc) => {
+        if (!shouldThrow)
+          return fs.writeSync(fd, buf, enc);
+
+        shouldThrow = false;
+
+        const err = new Error('EAGAIN')
+        err.code = 'EAGAIN'
+        throw err
+      }),
+    }
+  })
+
+  stream.on('ready', common.mustCall())
+
+  t.assert.ok(stream.write('hello world\n'))
+
+  shouldThrow = true;
+
+  t.assert.ok(stream.write('something else\n'))
+
+  stream.flushSync()
+  stream.end()
+
+  const { promise, resolve } = Promise.withResolvers();
+
+  stream.on('finish', common.mustCall(() => {
+    fs.readFile(dest, 'utf8', (err, data) => {
+      t.assert.ok(err === null)
+      t.assert.strictEqual(data, 'hello world\nsomething else\n')
+
+      resolve()
+    })
+  }))
+  stream.on('close', common.mustCall());
+
+  await promise
+})
+
+it('throw error in flushSync on EAGAIN', async (t) => {
+  const dest = file()
+  const fd = fs.openSync(dest, 'w')
+
+  let fsWriteCallCount = 0;
+
+  const errToThrow = new Error('EAGAIN')
+  errToThrow.code = 'EAGAIN'
+
+  const stream = new FastUtf8Stream({
+    fd,
+    sync: false,
+    minLength: 1000,
+    retryEAGAIN: common.mustCall((err, writeBufferLen, remainingBufferLen) => {
+      t.assert.strictEqual(err.code, 'EAGAIN')
+      t.assert.strictEqual(writeBufferLen, 12)
+      t.assert.strictEqual(remainingBufferLen, 0)
+      return false
+    }),
+    [kCustomFs]: {
+      ...fs,
+      writeSync: common.mustCallAtLeast((fd, buf, enc) => {
+        if (fsWriteCallCount > 0)
+          return fs.writeSync(fd, buf, enc)
+
+        fsWriteCallCount++
+
+        Error.captureStackTrace(errToThrow)
+        throw errToThrow
+      }),
+      fsyncSync: common.mustCall((...args) => {
+        return fs.fsyncSync.apply(null, args)
+      }),
+    },
+  })
+
+  stream.on('ready', common.mustCall())
+
+  t.assert.ok(stream.write('hello world\n'))
+  t.assert.throws(() => stream.flushSync(), errToThrow)
+
+  t.assert.ok(stream.write('something else\n'))
+  stream.flushSync()
+
+  stream.end()
+
+  const { promise, resolve } = Promise.withResolvers();
+
+  stream.on('finish', common.mustCall(() => {
+    fs.readFile(dest, 'utf8', (err, data) => {
+      t.assert.ok(err === null)
+      t.assert.strictEqual(data, 'hello world\nsomething else\n')
+
+      resolve()
+    })
+  }))
+  stream.on('close', common.mustCall())
+
+  await promise
+})
diff --git a/test/parallel/test-fastutf8stream-flush.js b/test/parallel/test-fastutf8stream-flush.js
index 8537ce9b87f..4aa3d738eb0 100644
--- a/test/parallel/test-fastutf8stream-flush.js
+++ b/test/parallel/test-fastutf8stream-flush.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-fsync.js b/test/parallel/test-fastutf8stream-fsync.js
index 9e98202d875..823b830cfa5 100644
--- a/test/parallel/test-fastutf8stream-fsync.js
+++ b/test/parallel/test-fastutf8stream-fsync.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-minlength.js b/test/parallel/test-fastutf8stream-minlength.js
index d92e75d2186..29bb3b3179d 100644
--- a/test/parallel/test-fastutf8stream-minlength.js
+++ b/test/parallel/test-fastutf8stream-minlength.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-mode.js b/test/parallel/test-fastutf8stream-mode.js
index 2b53a092a52..8db0dad036b 100644
--- a/test/parallel/test-fastutf8stream-mode.js
+++ b/test/parallel/test-fastutf8stream-mode.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-periodicflush.js b/test/parallel/test-fastutf8stream-periodicflush.js
index f2251fa81aa..f9d5de9ea4f 100644
--- a/test/parallel/test-fastutf8stream-periodicflush.js
+++ b/test/parallel/test-fastutf8stream-periodicflush.js
@@ -6,7 +6,7 @@ const tmpdir = require('../common/tmpdir');
 const { it } = require('node:test');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 
 tmpdir.refresh();
 process.umask(0o000);
diff --git a/test/parallel/test-fastutf8stream-reopen.js b/test/parallel/test-fastutf8stream-reopen.js
index 60abdc81446..d057ca0d9bb 100644
--- a/test/parallel/test-fastutf8stream-reopen.js
+++ b/test/parallel/test-fastutf8stream-reopen.js
@@ -5,7 +5,7 @@ const common = require('../common');
 const tmpdir = require('../common/tmpdir');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 const { it } = require('node:test');
 
 tmpdir.refresh();
diff --git a/test/parallel/test-fastutf8stream-retry.js b/test/parallel/test-fastutf8stream-retry.js
index 3d0e33d788a..ae5eae6d496 100644
--- a/test/parallel/test-fastutf8stream-retry.js
+++ b/test/parallel/test-fastutf8stream-retry.js
@@ -5,7 +5,7 @@ const common = require('../common');
 const tmpdir = require('../common/tmpdir');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 const { it } = require('node:test');
 
 tmpdir.refresh();
diff --git a/test/parallel/test-fastutf8stream-sync.js b/test/parallel/test-fastutf8stream-sync.js
index 3480362b837..d1c0e40f08c 100644
--- a/test/parallel/test-fastutf8stream-sync.js
+++ b/test/parallel/test-fastutf8stream-sync.js
@@ -5,7 +5,7 @@ const common = require('../common');
 const tmpdir = require('../common/tmpdir');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 const { it } = require('node:test');
 
 tmpdir.refresh();
diff --git a/test/parallel/test-fastutf8stream-write.js b/test/parallel/test-fastutf8stream-write.js
index 2d17d1f2d5f..be1913823c8 100644
--- a/test/parallel/test-fastutf8stream-write.js
+++ b/test/parallel/test-fastutf8stream-write.js
@@ -5,7 +5,7 @@ const common = require('../common');
 const tmpdir = require('../common/tmpdir');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 const { it } = require('node:test');
 
 tmpdir.refresh();
diff --git a/test/parallel/test-fastutf8stream.js b/test/parallel/test-fastutf8stream.js
index f4f7b681384..26ded5fe020 100644
--- a/test/parallel/test-fastutf8stream.js
+++ b/test/parallel/test-fastutf8stream.js
@@ -5,7 +5,7 @@ const common = require('../common');
 const tmpdir = require('../common/tmpdir');
 const fs = require('fs');
 const path = require('path');
-const FastUtf8Stream = require('internal/streams/fast-utf8-stream');
+const { FastUtf8Stream } = require('internal/streams/fast-utf8-stream');
 const { isMainThread } = require('worker_threads');
 const assert = require('assert');

I migrated just two tests (because I didn't had much time today) but I think with this we can migrate the other tests that depends on the proxyquire.

Off-topic: amazing PR, happy to see this being ported to the core to advance the logging in the core

Edit2: About the naming, I added some comments at H4ad/nodejs-logging-proposal#11 (comment) but in the end I think rename to something like TruncatingBufferedStream to be very clear about the behavior of drop.

@jasnell jasnell force-pushed the jasnell/port-sonicboom branch from 583dcfb to c6bd499 Compare July 12, 2025 20:23
@jasnell jasnell marked this pull request as ready for review July 12, 2025 20:24
@jasnell
Copy link
Member Author

jasnell commented Jul 12, 2025

@mcollina ... this is otherwise ready to land. We have a few suggestions for alternative names. Since this came from your code, I wanted to see if you have a preference on the naming before we land.

  • FastUtf8Stream
  • BufferedUtf8Stream
  • TrucatingBufferedStream
  • Something else...

@jasnell
Copy link
Member Author

jasnell commented Jul 13, 2025

I think for the name we'll go with FastUtf8Stream as named. Neither the BufferedUtf8Stream or TruncatingBufferedStream really capture the primary characteristic here, which is that the stream is intended to be fast a d utf8-only.

As a first step to porting portions of the pino structured
logger into the runtime, this commit ports the SonicBoom
module to the fs module as FastUtf8Stream.

This is a faithful port of the SonicBoom module with some
modern updates, such as converting to a Class and using
Symbol.dispose. The bulk of the implementation is unchanged
from the original.
@jasnell jasnell force-pushed the jasnell/port-sonicboom branch from c6bd499 to 40b4795 Compare July 13, 2025 18:28
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 13, 2025
@mcollina
Copy link
Member

I'm ok for FastUtf8Stream.

@jsumners
Copy link
Contributor

...really capture the primary characteristic here, which is that the stream is intended to be fast a d utf8-only.

I don't think "fast" ages well. It's also relative. 🤷‍♂️

@mcollina
Copy link
Member

Can we call it Utf8Stream? I think that would do.

@jsumners
Copy link
Contributor

Works for me.

@jasnell
Copy link
Member Author

jasnell commented Jul 15, 2025

SGTM. Now just need to figure out the flaky test failures in macos

@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 15, 2025
Copy link
Contributor

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fs: port SonicBoom module to fs module as FastUtf8Stream
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/16306591791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants