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

import-bundle tests hang under node <=v12.10, ok under >=v12.11 #837

Closed
warner opened this issue Apr 2, 2020 · 5 comments
Closed

import-bundle tests hang under node <=v12.10, ok under >=v12.11 #837

warner opened this issue Apr 2, 2020 · 5 comments
Assignees
Labels
import-bundle package: import-bundle

Comments

@warner
Copy link
Member

warner commented Apr 2, 2020

@Chris-Hibbert discovered that the new import-bundle module (which is the first to use async functions under tap) suffers from a hanging test when run under Node v12. It works fine under Node v13.

I'll investigate.

@warner warner self-assigned this Apr 2, 2020
@warner
Copy link
Member Author

warner commented Apr 2, 2020

I can reproduce this under v12.10.0 with two files:

// test.js
import './install-ses.js';
import bundleSource from '@agoric/bundle-source';
// install-ses.js
import { lockdown } from 'ses';
lockdown();

running with nvm exec 12.10.0 node -r esm test.js will hang unless I comment out the lockdown() call. It appears that merely importing bundle-source causes the hang, if done in a SES-tamed environment under Node 12.

@warner
Copy link
Member Author

warner commented Apr 2, 2020

Narrower: importing from rollup causes the hang, without anything else from bundle-source.

// test.js
import ' ./install-ses.js';
import { rollup } from 'rollup';

(remember this is all under -r esm)

@warner
Copy link
Member Author

warner commented Apr 3, 2020

.. except that running this in a new directory doesn't hang. Maybe it's one of the transitive dependencies.

@warner
Copy link
Member Author

warner commented Apr 3, 2020

In my original agoric-sdk tree, in the packages/import-bundle directory (where node_modules/ses is the latest 0.7.6), I can reproduce this with:

nvm exec 12.10.0 node -r esm
> const { lockdown } = require('ses')
> lockdown()
> require('rollup')

whereupon it hangs. When I use SIGINT to interrupt it, I get an error from Node's repl.js about Cannot add property prepareStackTrace, object is not extensible, which is expected (since I'd called lockdown()), but I don't know if it's just the SIGINT that made it want to prepare a stack trace, or if there was some earlier error involved.

In the same directory, if I omit the -r esm and run the same commands, it loads rollup just fine.

In a new directory, where I've installed mostly the same dependency versions, neither flavor hangs.

@warner
Copy link
Member Author

warner commented Apr 3, 2020

Bisecting on the version of Node.js, it appears that 12.10.0 is the last version that hangs. The test works starting with the very next release, 12.11.0.

This Node.js release upgrades the V8 engine from 7.6 to 7.7, and the V8 release notes mention the following changes to stack traces that might possibly be relevant:

Stack trace improvements

Almost all errors thrown by V8 capture a stack trace when they are created. This stack trace can be accessed from JavaScript through the non-standard error.stack property. The first time a stack trace is retrieved via error.stack, V8 serializes the underlying structured stack trace into a string. This serialized stack trace is kept around to speed up future error.stack accesses.

Over the last few versions we worked on some internal refactorings to the stack trace logic (tracking bug), simplifying the code and improving stack trace serialization performance by up to 30%.

The Node.js changelog also mentions the following items that I thought looked suspicious:

  • esm: make dynamic import work in the REPL (Bradley Farias) #29437
  • module: error for CJS .js load within type: module (Guy Bedford) #29492
  • module: reintroduce package exports dot main (Guy Bedford) #29494
  • src: print exceptions from PromiseRejectCallback (Anna Henningsen) #29513

@warner warner changed the title import-bundle tests hang under node v12, ok under v13 import-bundle tests hang under node <=v12.10, ok under >=v12.11 Apr 3, 2020
warner added a commit that referenced this issue Apr 15, 2020
We acquired a dependency on at least 12.11 (see #837) that we don't
understand enough to relax, and we're only testing CI on 13.

So we're declaring that we require Node 13 or higher. This updates the
top-level package.json's `engines:` property, so that `yarn` will complain
immediately if you use an old Node.

It also updates a SwingSet unit test to check for the new version, as a
backup.

closes #937
refs #837 (makes it obsolete)
closes #35
warner added a commit that referenced this issue Apr 15, 2020
We acquired a dependency on at least 12.11 (see #837) that we don't
understand enough to relax, and we're only testing CI on 13.

So we're declaring that we require Node 13 or higher. This updates the
top-level package.json's `engines:` property, so that `yarn` will complain
immediately if you use an old Node.

It also updates a SwingSet unit test to check for the new version, as a
backup.

closes #937
refs #837 (makes it obsolete)
closes #35
warner added a commit that referenced this issue Apr 15, 2020
We acquired a dependency on at least 12.11 (see #837) that we don't
understand enough to relax, and we're only testing CI on 12.16.1 (the current
LTS).

So we're declaring that we require Node 12.16.1 or higher. This updates the
top-level package.json's `engines:` property, so that `yarn` will complain
immediately if you use an old Node.

It also updates a SwingSet unit test to check for the new version, as a
backup.

closes #937
refs #837 (makes it obsolete)
closes #35
warner added a commit that referenced this issue Apr 15, 2020
We acquired a dependency on at least 12.11 (see #837) that we don't
understand enough to relax, and we're only testing CI on 12.16.1 (the current
LTS).

So we're declaring that we require Node 12.16.1 or higher. This updates the
top-level package.json's `engines:` property, so that `yarn` will complain
immediately if you use an old Node.

It also updates a SwingSet unit test to check for the new version, as a
backup.

closes #937
refs #837 (makes it obsolete)
closes #35
warner added a commit that referenced this issue Apr 15, 2020
…938)

We acquired a dependency on at least 12.11 (see #837) that we don't
understand enough to relax, and we're only testing CI on 12.16.1 (the current
LTS).

So we're declaring that we require Node 12.16.1 or higher. This updates the
top-level package.json's `engines:` property, so that `yarn` will complain
immediately if you use an old Node.

It also updates a SwingSet unit test to check for the new version, as a
backup.

closes #937
refs #837 (makes it obsolete)
closes #35
@katelynsills katelynsills added the import-bundle package: import-bundle label Feb 9, 2021
warner added a commit that referenced this issue May 20, 2021
warner added a commit that referenced this issue May 20, 2021
closes #1925
closes #837

The agoric-sdk as a whole requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 20, 2021
closes #1925
closes #837

The agoric-sdk as a whole requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 20, 2021
BREAKING CHANGE: The agoric-sdk as a whole now requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

closes #1925
closes #837

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 21, 2021
BREAKING CHANGE: The agoric-sdk as a whole now requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

closes #1925
closes #837

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 21, 2021
BREAKING CHANGE: The agoric-sdk as a whole now requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

closes #1925
closes #837

Co-authored-by: Mathieu Hofman <[email protected]>
@warner warner closed this as completed in f014684 May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-bundle package: import-bundle
Projects
None yet
Development

No branches or pull requests

2 participants