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

Buffer is not transferable in Node 21/22 #55593

Open
nex3 opened this issue Oct 29, 2024 · 15 comments
Open

Buffer is not transferable in Node 21/22 #55593

nex3 opened this issue Oct 29, 2024 · 15 comments
Labels
buffer Issues and PRs related to the buffer subsystem. web-standards Issues and PRs related to Web APIs

Comments

@nex3
Copy link

nex3 commented Oct 29, 2024

Version

v21.7.3, v22.11.0

Platform

Linux nweiz1 6.9.10-1rodete5-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.9.10-1rodete5 (2024-09-04) x86_64 GNU/Linux

Subsystem

worker_threads

What steps will reproduce the bug?

import {MessageChannel, Worker} from 'worker_threads';

const channel = new MessageChannel();
const buffer = Buffer.from('some text');
channel.port1.postMessage(buffer, [buffer.buffer]);
channel.port2.on('message', (e) => {
  console.log(Buffer.from(e).toString());
  channel.port1.close();
  channel.port2.close();
});

This worked in v20 and fails in v21 and v22.

How often does it reproduce? Is there a required condition?

It always reproduces.

What is the expected behavior? Why is that the expected behavior?

The buffer should be transferred through the message channel.

What do you see instead?

node:internal/per_context/domexception:53
    ErrorCaptureStackTrace(this);
    ^
DOMException [DataCloneError]: Cannot transfer object of unsupported type.
    at new DOMException (node:internal/per_context/domexception:53:5)
    at file:///.../test.mjs:5:15
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5)

Node.js v22.11.0

Additional information

I didn't see anything in the v21 or v22 changelogs about buffers becoming untransferable, and transferring it worked in v20, so I'm assuming this is unintentional. If it's intentional, I would have expected at least a deprecation message indicating that this was going to start breaking.

@nex3
Copy link
Author

nex3 commented Oct 29, 2024

Note that buffer.buffer is documented to be an ArrayBuffer, of which MDN says:

ArrayBuffer is a transferable object.

@avivkeller avivkeller added buffer Issues and PRs related to the buffer subsystem. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Oct 30, 2024
@avivkeller
Copy link
Member

Does this also occur in v23?

@nex3
Copy link
Author

nex3 commented Oct 30, 2024

Yes.

@avivkeller avivkeller added web-standards Issues and PRs related to Web APIs and removed v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Oct 30, 2024
@targos
Copy link
Member

targos commented Oct 30, 2024

I did a quick manual bisect using the nightly builds.
good: https://nodejs.org/download/nightly/v21.0.0-nightly20230505af9b48a2f1/
bad: https://nodejs.org/download/nightly/v21.0.0-nightly20230506a90a1459ee/

Diff: af9b48a...a90a145

Most likely caused by #47604

@bolt-juri-gavshin
Copy link

bolt-juri-gavshin commented Oct 31, 2024

Some additional context, if you replace this line...

channel.port1.postMessage(buffer, [buffer.buffer]);
  1. WIth channel.port1.postMessage(buffer); - it works
  2. WIth channel.port1.postMessage("Random string", [buffer.buffer]); - it still throws, buffer doesn't have to be posted to trigger the error

Also (executed right after buffer instantiation)...

console.log(isMarkedAsUntransferable(buffer.buffer)); //returns true
console.log(isMarkedAsUntransferable(new ArrayBuffer(10)));  //returns false

Does anybody know what's the plan, to make postMessage(buffer, [buffer.buffer]) work again or postMessage(buffer) will transfer it's buffer implicitly (it doesn't right now)?

@legendecas
Copy link
Member

There is an internal Buffer pool that can not be transfered. Buffers allocated in this pool are not transferable, as documented at https://nodejs.org/api/worker_threads.html#workermarkasuntransferableobject.

#47604 makes the "transfer" intent explicit that if the transfer can not be achieved, an error is thrown, according to the spec. The transfer operation was never completed without the change, instead, an implicit "copy" operation was performed.

I think this difference can be documented at https://nodejs.org/api/buffer.html#buffers-and-typedarrays as well.

@101arrowz
Copy link

Although the new behavior makes sense, it does make using buffers with worker threads less ergonomic.

When you allocate a Uint8Array using the de-facto constructor new Uint8Array(size), it's guaranteed to be transferable, but this isn't the case with dynamically allocated Buffer objects (or the results of fs.readFileSync, etc.) With the change that pooled Buffers cause DataCloneErrors on an attempted transfer, it has become more difficult to write code that takes advantage of transferring Uint8Array without explicitly handling the case that the Uint8Array is a pooled Buffer. This is mostly because it is difficult to even know if a given Buffer is pooled or not.

I've encountered the effects of this change in 101arrowz/fflate#227. The solution here seems to be to have my library retry the postMessage without any transferables on a DataCloneError, but it would be nice if Node could expose information about whether a given Buffer is transferable or not to users.

@legendecas
Copy link
Member

worker.isMarkedAsUntransferable(object) tells if an array buffer is transferable.

@101arrowz
Copy link

101arrowz commented Nov 12, 2024

Thanks, that's helpful! Are you familiar with any runtime agnostic way to detect transferability beyond just trying it and failing if it doesn't work? I think there's ArrayBuffer.prototype.detached that can detect if an array buffer has already been transferred, but this isn't available in Node from what I can tell. (Also it's not a perfect proxy for transferability anyway.)

@targos
Copy link
Member

targos commented Nov 12, 2024

ArrayBuffer.prototype.detached works fine in Node.js

@101arrowz
Copy link

101arrowz commented Nov 12, 2024

OK, MDN is just out of date then; might be worth bringing up with them. I'll just use isMarkedAsUntransferable for now though, as it doesn't look like ArrayBuffer.prototype.detached (or any other JS standard property that works across runtimes) specifically checks for non-transferability.

@bradzacher
Copy link

This is a pretty painful change. In v20 you can create a buffer and transfer it between workers, but in v22 it fails.

Using Buffer is provides a nice API to manipulate binary data -- eg I was using Buffer.concat to merge some buffers together after receiving some partial binary data via a stdin. Not being able to directly transfer this buffer to a worker thread for processing is a huge loss for performance-critical applications.

@101arrowz
Copy link

As I understand the behavior before wasn't actually transferring the data anyway, it was just silently cloning it even if it were placed in the transferables array, so this change doesn't have any negative performance impact. You can use Buffer.allocUnsafeSlow and manually copy your chunks in, then you can get a Buffer that is guaranteed transferable.

@bradzacher
Copy link

bradzacher commented Nov 15, 2024

That's quite interesting! I didn't realise that was the behaviour. When I added it to the transferable I assumed I was being performant 😅

Well then is there any way to get binary data from NodeJS builtin APIs as a Buffer (eg from fs or stdin) and transfer it to another thread without any copying at all? I suspect that there's a lot of people that want to do this sort of behaviour with Buffer.

For context my usecase is that I have a server of sorts which is receiving binary data via stdin and I forward the data to one of the worker threads for handling - so being able to pass the bytes from stdin to whichever worker is free without copying would be ideal to improve throughput.

@legendecas
Copy link
Member

Buffers from stdin are capable of being transfered.

Only buffers created with Buffer.allocUnsafe(), Buffer.from(array), Buffer.from(string), and Buffer.concat() may use the buffer pool and not capable of being transfered. This behavior can be disabled with Buffer.poolSize = 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. web-standards Issues and PRs related to Web APIs
Projects
None yet
Development

No branches or pull requests

7 participants