GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — contextify: tie lifetimes of context & sandbox by ofrobots · Pull Request #5786 · nodejs/node

Conversation

@ofrobots

@ofrobots ofrobots commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

contextify

Description of change

When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

Fixes: #5768
R=@bnoordhuis
/cc @evanlucas @cjihrig

Note that this is not trivially back-portable to 5.x. SetPrivate doesn't exist on 5.x, but I can create a new PR once this lands on master. This does continue to address the memory growth issue reported in #3113, but we should get this vetted before backporting.

CI: https://ci.nodejs.org/job/node-test-pull-request/1960/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Mar 18, 2016
@cjihrig cjihrig mentioned this pull request Mar 18, 2016
Revert recent contextify changes #5782
Closed
@MylesBorins

MylesBorins commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

@ofrobots since this will take time to backport to v5.x do you think we should be keeping #5782 open and cutting a v5.9.1 with the changes reverted?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 18, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@cjihrig

cjihrig commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ofrobots

ofrobots commented Mar 18, 2016

Copy link
Copy Markdown
Contributor Author

I want to avoid a deeper hole, specially that this is going to be a Friday release. If bugs in this code still remain, they are going to be gc (i.e. timing) related. I think it would be best to proceed with a revert (i.e. #5782) and allow in depth review on this PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Knighton910

Knighton910 commented Mar 18, 2016

Copy link
Copy Markdown

+1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
trevnorris
trevnorris reviewed Mar 18, 2016
View reviewed changes
Comment thread test/parallel/test-vm-create-and-run-in-context.js Outdated

@trevnorris trevnorris Mar 18, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: other two locations where --expose-gc is used the comment is placed directly after the 'use strict';.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ofrobots ofrobots Mar 19, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ofrobots
contextify: tie lifetimes of context & sandbox
95a70d9 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

Fixes: nodejs#5768
PR-URL: nodejs#5786
@ofrobots
ofrobots force-pushed the fix-5768 branch from e18386e to 95a70d9 Compare March 19, 2016 03:12
@trevnorris

trevnorris commented Mar 19, 2016

Copy link
Copy Markdown
Contributor

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@bnoordhuis

bnoordhuis commented Mar 19, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ofrobots added a commit to ofrobots/node that referenced this pull request Mar 19, 2016
@ofrobots
contextify: tie lifetimes of context & sandbox
5940430 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of nodejs#5786 to v5.x.

Ref: nodejs#5786
@ofrobots ofrobots mentioned this pull request Mar 19, 2016
Backport 5786 to v5.x #5800
Closed
3 tasks done
ofrobots added a commit that referenced this pull request Mar 19, 2016
@ofrobots
contextify: tie lifetimes of context & sandbox
a53b2ac 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

Fixes: #5768
PR-URL: #5786
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: cjihrig - Colin Ihrig <[email protected]>
Reviewed-By: trevnorris - Trevor Norris <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@ofrobots

ofrobots commented Mar 19, 2016

Copy link
Copy Markdown
Contributor Author

Thanks. Landed in a53b2ac. PR for backport to v5.x is here: #5800.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ofrobots ofrobots closed this Mar 19, 2016
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@ofrobots Myles Borins
contextify: tie lifetimes of context & sandbox
c5d8369 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of #5786 to v5.x.

Ref: #5786
PR-URL: #5800
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Mar 22, 2016
v5.9.1 Release Proposal #5831
Merged
@Fishrock123 Fishrock123 added the dont-land-on-v5.x label Mar 22, 2016
@MylesBorins MylesBorins added the dont-land-on-v4.x label Mar 23, 2016
@rvagg rvagg mentioned this pull request May 19, 2016
Backport 5786 to v4.x (memory growth with 'vm' module) #6871
Merged
rvagg pushed a commit to rvagg/io.js that referenced this pull request May 20, 2016
@ofrobots @rvagg
contextify: tie lifetimes of context & sandbox
c6db822 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of nodejs#5786 to v5.x.

Ref: nodejs#5786
PR-URL: nodejs#5800
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 20, 2016
@ofrobots Myles Borins
contextify: tie lifetimes of context & sandbox
823f726 
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

This is a backport of #5786 to v5.x.

Ref: #5786
PR-URL: #5800
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

vm: recent contextify changes cause segmentation fault in node v5.9

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@ofrobots @MylesBorins @jasnell @cjihrig @Knighton910 @trevnorris @bnoordhuis @mscdex @Fishrock123
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.