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

chore(ci): test on node v16 #436

Merged
merged 3 commits into from
May 6, 2021
Merged

chore(ci): test on node v16 #436

merged 3 commits into from
May 6, 2021

Conversation

SimenB
Copy link
Collaborator

@SimenB SimenB commented Apr 22, 2021

No description provided.

@github-actions
Copy link

Thanks for opening this pull request! Each pull request require an update in the CHANGELOG. Please update it based on your changes.

@SimenB
Copy link
Collaborator Author

SimenB commented Apr 22, 2021

Oh wow, our tests make node abort 🙈

/cc @sam-github @siimon @zbjornson

I guess we should try to add this module to CITGM after we figure out what's wrong

@zbjornson
Copy link
Collaborator

node[1757]: ../src/env-inl.h:1052:void node::Environment::AddCleanupHook(node::Environment::CleanupCallback, void*): Assertion `(insertion_info.second) == (true)' failed.
 1: 0xb12b00 node::Abort() [node]
 2: 0xb12b7e  [node]
 3: 0xb7a58c  [node]
 4: 0xd5f70b  [node]
 5: 0xd60bac  [node]
 6: 0xd61226 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 7: 0x160c579  [node]
Aborted (core dumped)

Not particularly helpful. I can try to take a look later this week/weekend.

@analytik
Copy link

analytik commented Apr 26, 2021

I got the same error when experimenting with a fix for #435 ... currently looking for a quickfix, even if not elegant.

EDIT: this happens when you try to call perf_hooks.performance.measure(someValue) several times with the same someValue it seems, at least for 'gc'.

EDIT 2: see correction below, the bug is on a (new perf_hooks.PerformanceObserver(...)).observe(...)

@SimenB
Copy link
Collaborator Author

SimenB commented Apr 26, 2021

@analytik do you have a minimal reproduction? I tried this and it seems to work?

const { performance } = require("perf_hooks");

for (let i = 0; i < 10; i++) {
  performance.measure("gc");
}

@analytik
Copy link

analytik commented Apr 26, 2021

I think I got it - see my fresh bug report in node.js

EDIT: I misspoke, not .measure(), but .observe() for a new perf_hooks.PerformanceObserver instance

@SimenB
Copy link
Collaborator Author

SimenB commented May 4, 2021

16.1.0 released with a fix

@SimenB
Copy link
Collaborator Author

SimenB commented May 5, 2021

16.1.0 not added here yet, so 16.x does not work

@SimenB SimenB mentioned this pull request May 5, 2021
4 tasks
@analytik
Copy link

analytik commented May 5, 2021

Well, at least I can confirm that on Windows, 16.1.0 passes all tests and 16.0.0 crashes.

@SimenB
Copy link
Collaborator Author

SimenB commented May 5, 2021

Yeah, CI passes when I force it to be 16.1 👍

@SimenB SimenB merged commit c7b9a9d into master May 6, 2021
@SimenB SimenB deleted the SimenB-patch-1 branch May 6, 2021 22:22
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 this pull request may close these issues.

None yet

3 participants