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

Unable to deserialize cloned data #49844

Closed
H4ad opened this issue Sep 24, 2023 · 9 comments · Fixed by #50026
Closed

Unable to deserialize cloned data #49844

H4ad opened this issue Sep 24, 2023 · 9 comments · Fixed by #50026

Comments

@H4ad
Copy link
Member

H4ad commented Sep 24, 2023

Version

main

Platform

Linux h4ad 5.15.0-82-generic #91~20.04.1-Ubuntu SMP Fri Aug 18 16:24:39 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

js_transferable

What steps will reproduce the bug?

To reproduce, build the NodeJS on the main branch, add the following benchmark inside perf_hooks folder with the name of histogram.js:

'use strict';

const assert = require('assert');
const common = require('../common.js');

const { createHistogram } = require('perf_hooks');

const bench = common.createBenchmark(main, {
  n: [1e4],
  operation: ['creation', 'clone'],
});

let _histogram;

function main({ n, operation }) {
  switch (operation) {
    case 'creation': {
      bench.start();
      for (let i = 0; i < n; i++)
        _histogram = createHistogram();
      bench.end(n);

      // Avoid V8 deadcode (elimination)
      assert.ok(_histogram);
      break;
    }
    case 'clone': {
      bench.start();
      for (let i = 0; i < n; i++)
        _histogram = structuredClone(createHistogram());
      bench.end(n);

      // Avoid V8 deadcode (elimination)
      assert.ok(_histogram);
      break;
    }
    default:
      throw new Error(`Unsupported operation ${operation}`);
  }
}

Then run the benchmark with the command:

node benchmark/compare.js --filter histogram.js --old ./out/Release/node --new ./out/Release/node perf_hooks

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

This error doesn't throw every time you call structuredClone, you will be able to find it more frequently running the benchmark, if you run the benchmark directly, without compare.js, you will need to run at least 10x to be able to see the issue, is very weird.

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

The cloning always works, instead of throwing errors randomly.

What do you see instead?

"new","perf_hooks/histogram.js","operation='creation' n=10000",94286.57932467105,0.106059633
node:internal/worker/io:409
  const message = receiveMessageOnPort_(port?.[kHandle] ?? port);
                  ^

Error: Unable to deserialize cloned data.
    at receiveMessageOnPort (node:internal/worker/io:409:19)
    at structuredClone (node:internal/structured_clone:24:10)
    at main (/home/h4ad/Projects/opensource/node-copy-3/benchmark/perf_hooks/histogram.js:30:22)
    at /home/h4ad/Projects/opensource/node-copy-3/benchmark/common.js:54:9
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Node.js v21.0.0-pre

Additional information

I did a bisect and this issue was introduced by this commit 38dee8a.

So, @legendecas, maybe you have some hints about what is happening here.

@joyeecheung
Copy link
Member

This could be what we have been seeing in the CI too #49852

@legendecas
Copy link
Member

The unstable behavior is caused by the weak global handles held by JSTransferables. When deserializing, a new JS object and a new JSTransferable wrapper are created for the serialized data. However, the JSTransferable's global handles for both the wrapper object and target object are weak so JSTransferable::target() may return an empty result. V8 ValueDeserializer would throw in the case of an empty deserialization result.

#50026 makes the JSTransferable global handles to be strong references. Since JSTransferables are created in place for serialization or deserialization, strong references would be released once the process is completed with detached BaseObjectPtrs. This avoids the newly created objects in the deserialization being garbage-collected and resulting in errors.

nodejs-github-bot pushed a commit that referenced this issue Oct 10, 2023
JSTransferable wrapper object is a short-lived wrapper in the scope of
the serialization or the deserialization. Make the JSTransferable
wrapper object pointer as a strongly-referenced detached BaseObjectPtr
so that a JSTransferable wrapper object and its target object will never
be garbage-collected during a ser-des process, and the wrapper object
will be immediately destroyed when the process is completed.

PR-URL: #50026
Fixes: #49852
Fixes: #49844
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
JSTransferable wrapper object is a short-lived wrapper in the scope of
the serialization or the deserialization. Make the JSTransferable
wrapper object pointer as a strongly-referenced detached BaseObjectPtr
so that a JSTransferable wrapper object and its target object will never
be garbage-collected during a ser-des process, and the wrapper object
will be immediately destroyed when the process is completed.

PR-URL: nodejs#50026
Fixes: nodejs#49852
Fixes: nodejs#49844
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
JSTransferable wrapper object is a short-lived wrapper in the scope of
the serialization or the deserialization. Make the JSTransferable
wrapper object pointer as a strongly-referenced detached BaseObjectPtr
so that a JSTransferable wrapper object and its target object will never
be garbage-collected during a ser-des process, and the wrapper object
will be immediately destroyed when the process is completed.

PR-URL: nodejs#50026
Fixes: nodejs#49852
Fixes: nodejs#49844
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@mividtim
Copy link

mividtim commented Jun 7, 2024

👋🏻 Hi, y'all. Quick question. This is a bug in the LTS version of Node. Shouldn't this bug fix make its way into a Node 20 release? Node 21 shouldn't be used in production, and Node 22 doesn't enter LTS until late October. And one of the native packages I use in production doesn't have support for Node 22, yet.

@richardlau
Copy link
Member

This is a bug in the LTS version of Node. Shouldn't this bug fix make its way into a Node 20 release? Node 21 shouldn't be used in production, and Node 22 doesn't enter LTS until late October.

According to the description this was bisected to 38dee8a which is #47956 and isn't in Node.js 20.

@mividtim
Copy link

mividtim commented Jun 7, 2024

Thanks for the quick reply, @richardlau! I see this error message consistently in Node 20.12.0 in a Node test runner test. Could I be encountering a different bug with the same symptom?

@richardlau
Copy link
Member

Possibly.

@mividtim
Copy link

mividtim commented Jun 7, 2024

Hmm... Whatever it was seems to have been fixed. Bumping to Node 20.14.0 resolved the issue, which was consistent in 20.12.0. Anyway, thanks for the quick reply. I appreciate it!

@Aliendie Aliendie mentioned this issue Jun 7, 2024
@fraxken
Copy link
Member

fraxken commented Jun 30, 2024

The issue still occur with the test_runner on Node.js 20.15 or Node.js 22

image

But that's quite hard to reproduce or get a minimal example

@legendecas
Copy link
Member

@fraxken the error shown in the screenshot is about the snapshot was taken in a Node.js version different than the current running one. Please file a new issue if you can reproduce it with single Node.js version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants