fflate icon indicating copy to clipboard operation
fflate copied to clipboard

fflate and Node v22

Open HowardBraham opened this issue 1 year ago • 8 comments

How to reproduce

The fflate library appears to be incompatible with Node v22, now the LTS version.

It fails and hangs in our application.

The problem

Running the test suite in Node v22 produces this result:

$ TS_NODE_PROJECT=test/tsconfig.json uvu -b -r ts-node/register test
0-valid.ts
 compression  ✘ • • • •   (4 / 5)

   FAIL  compression  "basic"
    Cannot transfer object of unsupported type.

    at Object.deflate (/workspaces/fflate/test/util.ts:122:20)
    at Object.<anonymous> (/workspaces/fflate/test/0-valid.ts:10:34)
    at step (/workspaces/fflate/test/0-valid.ts:33:23)
    at Object.next (/workspaces/fflate/test/0-valid.ts:14:53)
    at /workspaces/fflate/test/0-valid.ts:8:71
    at new Promise (<anonymous>)
    at __awaiter (/workspaces/fflate/test/0-valid.ts:4:12)
    at Object.compression (/workspaces/fflate/test/0-valid.ts:46:16)


error Command failed with exit code 1.

HowardBraham avatar Nov 08 '24 21:11 HowardBraham

That's a bit concerning - I'll have a look tomorrow.

101arrowz avatar Nov 09 '24 23:11 101arrowz

OK, this is just a bug in the test-bench because I was trying to transfer a Buffer with allocUnsafe (which can return a reference to a non-transferable buffer pool), instead of allocUnsafeSlow (which avoids using the the pool at all).

Is there a larger issue you're running into with Node v22 that causes the hanging behavior you've described?

101arrowz avatar Nov 11 '24 18:11 101arrowz

We use this in the MetaMask Extension's build system and discovered it when attempting to update to Node v22 (our issue: https://github.com/MetaMask/metamask-extension/pull/28368)

We use it here: https://github.com/MetaMask/metamask-extension/blob/39528b02100a6003f412bbef5e0b560002c945bc/development/webpack/utils/plugins/ManifestPlugin/index.ts#L48-L63 (ignore the "use a copy of the Buffer" comment, the Buffer copy operation is done elsewhere)

davidmurdoch avatar Nov 19 '24 18:11 davidmurdoch

I'm not sure if it helps, but if you add this test to your suite, it passes on Node 20 but fails on Node 22. It also passes if you use ZipDeflate instead of AsyncZipDeflate.

import { testSuites } from "./util";
import { AsyncZipDeflate, Zip } from '../src/index';

testSuites({
  async asyncZipDeflate(file) {
    const zip = new Zip((err, dat, final) => {
      if (!err) {
        console.log(dat, final);
      }
    });

    const helloTxt = new AsyncZipDeflate('hello.txt', {
      level: 0
    });

    zip.add(helloTxt);
    helloTxt.push(file, true);
    zip.end();
  }
});

HowardBraham avatar Nov 20 '24 18:11 HowardBraham

@101arrowz any ideas how to solve this?

HowardBraham avatar Nov 26 '24 00:11 HowardBraham

@davidmurdoch @HowardBraham I was checking this out, given I'm about to add fflate as dependency to one of my projects, and we definitely want support to v22, and I noticed that the test you have is wrong, you are passing a Buffer instead of Uint8Array to helloTxt.push(), try converting it to Uint8Array and see how that goes, same on the metamask-extension codebase! You can do that with fflate.strToU8(file.toString()) :slightly_smiling_face:

BTW, the change of allocUnsafe to allocUnsafeSlow is still needed, but that's just some "test suite" helper of the package.

pedrokehl avatar Dec 12 '24 19:12 pedrokehl

In Node.js instances of Buffer are also instances of UInt8Array: https://nodejs.org/docs/latest-v22.x/api/buffer.html#buffer

The Buffer class is a subclass of JavaScript's Uint8Array class and extends it with methods that cover additional use cases.

davidmurdoch avatar Dec 12 '24 20:12 davidmurdoch

The real issue here is with the tests that Node.js has decided to disable the old behavior of silently allowing attempted transfers of buffers that are views into the global Buffer pool. Transferring those buffers was never possible (otherwise all pooled buffers would be invalidated - not good) but fflate assumed it was fine to attempt to transfer them because I thought that the Node runtime would just copy the memory on an attempted transfer of a pooled Buffer. I thought this was a reasonable assumption since pooling is an implementation detail of Node.js, and IMO it really is the responsibility of Node to deal with the consequences of that decision.

My assumption was accurate until Node.js version 22; now however, Node throws an error when you attempt to transfer a pooled Buffer object. That's what the linked issue discusses.

This issue does not affect any synchronous functions because no data needs to be transferred between threads. It also doesn't affect most of the asynchronous functions because they do not assume consume the input by default (i.e. they copy memory to the other thread instead of transferring). However, it does affect the streaming asynchronous APIs, including AsyncZipDeflate and cousins, because those do consume the input (transfer memory instead of copying) by default.

What I'm planning to do is special-case the Node.js worker implementation to skip transferring buffers that are pooled altogether, which will essentially restore the old functionality. For now, you can work around the issue by copying all buffers before passing them into a streaming asynchronous API:

const myZipDeflate = new fflate.AsyncZipDeflate(...);.

const data = fs.readFileSync('hello.txt');

// instead of this:
// myZipDeflate.push(data)

// do this:
myZipDeflate.push(new Uint8Array(data));

// or this, if you want to save some copies:
const { isMarkedAsUntransferable } = require('worker_threads');

let transferableBuffer = isMarkedAsUntransferable(data) ? new Uint8Array(data) : data;
myZipDeflate.push(transferableBuffer);

(BTW: the reason "converting" a Buffer to Uint8Array avoids the issue is that by doing so you reallocate the memory into a non-pooled allocation that can be safely transferred.)

101arrowz avatar Dec 12 '24 21:12 101arrowz