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

src: avoid making JSTransferable wrapper object weak #50026

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

legendecas
Copy link
Member

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.

Fixes: #49852
Fixes: #49844

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.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 3, 2023
}

JSTransferable::~JSTransferable() {
HandleScope scope(env()->isolate());
Copy link
Member

Choose a reason for hiding this comment

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

For education purposes: What is the purpose of initializing a scope in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

A local handle is created with target_.Get(env()->isolate()) in the next line. A handle scope is needed in place to collect the local handle when it goes out of lexical scope.

@H4ad
Copy link
Member

H4ad commented Oct 6, 2023

The crash still occurs even with this commit:

[00:00:08|%   7| 0/1 files |  4/60 runs | 1/2 configs]: perf_hooks/histogram.js node:internal/worker/io:410
  const message = receiveMessageOnPort_(port?.[kHandle] ?? port);
                  ^

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

Node.js v21.0.0-pre

@legendecas
Copy link
Member Author

Stress test: https://ci.nodejs.org/job/node-stress-single-test/459/

I can reproduce with pummel/test-structuredclone-jstransferable locally on the main branch in 10 runs, and no reproduction with the patch.

@legendecas
Copy link
Member Author

@H4ad the stress test passed. Would you mind sharing more information about your local test? Thank you.

@H4ad
Copy link
Member

H4ad commented Oct 6, 2023

@legendecas Try with this code:

'use strict';

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

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

const bench = common.createBenchmark(main, {
  n: [1e5],
  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}`);
  }
}

Put it inside benchmark/perf_hooks and then run with this new patch.

@legendecas
Copy link
Member Author

legendecas commented Oct 6, 2023

Note that benchmark/compare.js runs the benchmark with the new node binary and the old node binary. So it would report errors for node binary without this patch. Would you mind verifying if the error happened with the new node binary? Because from what I can tell from your posted output result, it seems like you are running with benchmark/compare.js. Please correct me if I'm getting wrong.

@H4ad
Copy link
Member

H4ad commented Oct 6, 2023

Oh, you are right, I forgot to compile the old version with your patch, sorry about that.

In this case, I can confirm this PR fixes my bug! Thanks for your work!

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2023
@nodejs-github-bot nodejs-github-bot merged commit 78a1570 into nodejs:main Oct 10, 2023
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 78a1570

@legendecas legendecas deleted the js_transferable_weak branch October 10, 2023 07:50
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request 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>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 11, 2023
debadree25 pushed a commit to debadree25/node that referenced this pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wpt/test-structured-clone is flaky Unable to deserialize cloned data
6 participants