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

support node 14 #723

Closed
wants to merge 5 commits into from
Closed

support node 14 #723

wants to merge 5 commits into from

Conversation

koonpeng
Copy link
Collaborator

This is an ongoing work to support node >= v14.

node v14's version of v8 has changed the API for ArrayBuffer::New which cause rclnodejs to stop working. Instead of using createArrayBufferFromAddress, this PR changes to use type array's constructor to create a view of the underlying buffer. As a side effect, there should also no longer be a need to manually free the underlying buffer, v8 should automatically free it when there is no more references.

Tests are failing on node >= v14.9 due to nodejs/node#35620. And is failing on < v14.9 possibly due to lovell/sharp#2196. The fix for the former should be released in the next version so I will put this on hold until that happens.

TODO: Test for leaks with asan.

@koonpeng koonpeng marked this pull request as draft November 16, 2020 06:17
@minggangw
Copy link
Member

So I reckon this PR could also work properly on node < 14 (before change of ArrayBuffer::New), right?

@koonpeng
Copy link
Collaborator Author

I don't see any reason for it to break on older versions, but I haven't tested it yet.

I'm looking at how I can make automated asan test easier, I'm getting lots of noise from other modules currently.

@minggangw
Copy link
Member

Great! 👍

@coveralls
Copy link

coveralls commented Dec 1, 2020

Coverage Status

Coverage remained the same at 91.747% when pulling c924a36 on fix/node-14 into 6f8a1a1 on develop.

@minggangw
Copy link
Member

Is it possible that the crash is caused by node-ffi-napi, please check out node-ffi-napi/ref-napi#47

@koonpeng
Copy link
Collaborator Author

koonpeng commented Jan 7, 2021

Is it possible that the crash is caused by node-ffi-napi, please check out node-ffi-napi/ref-napi#47

It does look like the same bug, hopefully it will work be fixed in the next release of ref-napi. When that happens I will test if the changes in this PR is needed, in the mean time I think I will continue leaving this as a draft.

@minggangw
Copy link
Member

Hope so, thanks!

@csmith-rtr
Copy link

@minggangw @koonpeng are you planning to complete the transition to Node v14? Support for Node v12 is ending in April 2022. At this point, it might make more sense to go straight to v16.

@wayneparrott
Copy link
Collaborator

@csmith-rtr Work on this has been progressing. See the node-16 branch https://github.com/RobotWebTools/rclnodejs/tree/nodejs-16 that is under development. I've been running it for the past 2 weeks with node 12 & 16 on foxy and galactic with no obvious issues. Hopefully it will merge to head (develop branch) soon.

@csmith-rtr
Copy link

great! I hadn't seen that branch.

@minggangw minggangw closed this Jun 20, 2022
@minggangw minggangw deleted the fix/node-14 branch June 20, 2022 03:04
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.

5 participants