Node-API migration #2305

GazHank · 2021-08-06T18:33:35Z

DRAFT PR - FOR REVIEW ONLY

This is an initial draft of the migration to NAPI, inaddition to the automated migration tool quite a lot of the migration has to be hand crafted. So far only the core / architecture agnostic functionality has been updated.

I have tried to keep all changes:

as small as possible
replacing logic like for like
applying the same changes throughout for consistency

As such some enhancement, improvements or simplifications that we gain from switching to NAPI may not be present (e.g. revised error handling)

Changes:

Automated conversion to NAPI

SerialPort.cpp

Update type conversions not handled automatically (we also need to decide if we retain int32 or just switch to int64 now)
String conversions updated
Add missing return types
Switch uv_work_t to napi_async_work (uv_queue_work to napi_queue_async_work)
Switch return argv (array) to args (vector)
Revise delete to napi_delete_async_work and free
Avoid uncheck napi_* calls for return args (per feedback from @KevinEady)
Replace napi_create_async_work with Napi::AsyncWorker (per feedback from @KevinEady and @NickNaso)

Serialport.h

Change main methods to return Napi::Value and consume const Napi::CallbackInfo& info
Change EIO methods to consume napi_env env, void* req
Change EIO_After methods to consume napi_env env, napi_status status, void* req
Remove workaround for Electron 11, as this version is no longer supported ~~Replace API version check with ABI stable runtime check~~
Change structs:
- Remove all references to AsyncResource (currently commented out)
- Switch callback to use Napi::FunctionReference
- Include napi_async_work work attribute
- Replace AsyncResource with Napi::AsyncWorker (per feedback from @KevinEady and @NickNaso)

Serialport_win

Same updates as serialport.cpp
Replace napi_create_async_work with Napi::AsyncWorker (common methods)
Replace napi_create_async_work with Napi::AsyncWorker (list)
Replace uv_async_t with Napi::AsyncWorker (read & write)
Fix event handling (close / disconnect event)

poller

Automated conversion
Manual conversion
fix destroy ()

darwin_list

Same updates as serialport.cpp
Replace napi_create_async_work with Napi::AsyncWorker (list)

binding.gyp

Use include_dir instead of include. (per feedback from @NickNaso)

Please note: This should now be ready for testing. Please let me know if you encounter any issues or unexpected behaviour

cinderblock · 2021-08-19T00:51:25Z

What is needed to get this over the edge? Or is this possibly complete and just needs reviews and approval? I'd love to have this working or test with it.

reconbot

I am not qualified to review this for c++ correctness but the logic doesn't seem to have changed. This is amazing.

You'll only need the napi package in the bindings package, you can leave the others alone.

I think we'll have to take a look at what prebuild can do to take advantage of napi too.

.generators/package.json

packages/binding-abstract/package.json

reconbot · 2021-08-19T03:11:01Z

@GazHank what do you mean by

As such some enhancement, improvements or simplifications that we gain from switching to NAPI may not be present (e.g. revised error handling)

What's the error handling?

Apologies for not reviewing this sooner, I was busy with work and life. I'm so happy to see this PR.

reconbot · 2021-08-19T03:17:16Z

So the poller is funny. I don't think that NAPI will give you anything you need to do the actual fs polling. However I at one time got assurances from the libuv team that they are ABI stable and will not change libuv's abi until their next major version. So we probably don't have to worry about until LIBUV 2.0 and just replace the nan stuff.

cinderblock · 2021-08-19T04:04:24Z

Why does poller.h still #include <nan.h> @GazHank?

cinderblock · 2021-08-19T04:06:35Z

@reconbot, slightly off topic, have you considered adding coverage tests for the C/C++ sources?

reconbot · 2021-08-19T04:25:03Z

@cinderblock he's still working on this it's a work in progress.

I have a suite of integration tests that work against actual hardware. I don't have coverage stats of the c++ however.

GazHank · 2021-08-19T20:00:44Z

@reconbot no worries on the timing, I feel you on the "other commitments"; I hope to be able to get some quality time on this again in the next few weeks, if I can juggle those other pesky priorities :-)

@cinderblock, et al

I'll try to address the questions and discussions in the chain, but am away from my dev machine at the moment, so appologies if some of the items will need follow up:

What is needed to get this over the edge:

I've tried to reflect the status / progress in the task list within the first comment and update this as I make further changes;
In addition to the items listed, we will of course need to test on each platform (while I'm able to perform testing in Windows and Linux, I will need help to test on Mac, and with a wider selection of hardware)

You'll only need the napi package in the bindings package, you can leave the others alone.

Nice spot

What's the error handling?

I believe when I was reviewing the N-API docs the handling of errors seemed simpler and easier than NAN, I'd have to look up the specifics, but should also flag that at present I've avoided any such refactoring, and soley focused on a like for like migration where possible

Why does poller.h still #include <nan.h>

Yes, it's because it's a WIP, and the poller files on my local machine have only been processed via the automated coversion tool so far, so will not compile or run for a few reasons
If others are able to help review and refine the code then I'm happy to commit those WIP files, but had planned to hold them until I had time to manually refactor the files

So the poller is funny. I don't think that NAPI will give you anything you need to do the actual fs polling. However I at one time got assurances from the libuv team that they are ABI stable and will not change libuv's abi until their next major version. So we probably don't have to worry about until LIBUV 2.0 and just replace the nan stuff.

I will have to take a look at some of the draft changes on my Linux machine, but from memory I think you are right.
The way that I have tried tweaking (rather than converting) the logic within the Win10 read and write methods, was intended to be a bit of a trial for the approach I was considering for updating the poller. If I can get those working then I think I can replicate some of the changes on that side, Although the size of the change on the poller side is a fair bit bigger, so there may be additional changes needed that I haven't yet reviewed.

My main interest in getting N-API experts to help with the review is to make sure I'm not reinventing the wheel on any of this stuff :-D

Please feel free to query, comment and suggest changes, they are very welcome

cinderblock · 2021-08-19T22:17:12Z

@reconbot Are the GitHub Actions running on hardware that has a serial port to test against?

Assuming yes, or even if not, the tests that are run here seem to run against all the target platforms.

If fail-fast were set to false, broken platforms wouldn't prevent testing of non-broken platforms.

KevinEady

Hello from the Node-API team! This is a great start to a migration. Please see my below comments. The main concern I see is unchecked used of napi_* calls: when calling into the engine, you need to validate the status of napi_ok before making another call. The node-addon-api library does this under the hood, so if you can use the library instead of the the underlying napi_* calls, then you wouldn't need to add those checks yourself. This would mean using an AsyncWorker instead of napi_async_work. Let us know if you have any questions!

packages/bindings/src/darwin_list.cpp

GazHank · 2021-08-21T09:56:19Z

Thanks @KevinEady that's super helpful, I'll try to action those changes (I think the napi_ change will be in quite a few places, so will look to update those all at once if I can).

I'm still to post the migrated poller.c and poller.h as following the use of the migration tool they still need a lot of work. Would you be able to take a quick look at those file and let me know if there are any previous migrations of projects using similar functionality?

reconbot · 2021-08-21T13:04:29Z

@GazHank i think logical changes in commits and keep everything on one branch. Is ok 👍 this can be a big pr, it's hard to do this piecemeal.

NickNaso

Hi everybody,
this is a good start for the migration to Node-API. like suggested by @KevinEady it could be a good idea to use Napi::AsyncWorker instead of the C API.
For example the following structure:

struct VoidBaton {//: public Napi::AsyncResource {
  VoidBaton() : // AsyncResource("node-serialport:VoidBaton"), 
  errorString() {}
  int fd = 0;
  Napi::FunctionReference callback;
  napi_async_work work;
  char errorString[ERROR_STRING_SIZE];
};

could became an Napi::AsyncWorker:

struct VoidBaton : public Napi::AsyncWorker {
  VoidBaton(Napi::Function& callback, int fd) : Napi::AsyncWorker(callback, "your resource name"), fd(fd)
  errorString() {}
  int fd = 0;
  char errorString[ERROR_STRING_SIZE];
};

Thrn you should to implement the Napi::AsyncWorker::Execute method to do the task you need and handle the result ny mtehods Napi::AyncWorker::OnOK and Napi::AsyncWorker::OnError.

packages/bindings/binding.gyp

packages/bindings/src/serialport.cpp

GazHank · 2021-08-23T18:29:43Z

Thank you all for the comments and feedback, it is very greatly appreciated. I've incorporated most of the feedback so far, and hope to get some time tomorrow to look into the switch to Napi::AsyncWorker as this seems to be a potentially larger and more significant change.

GazHank · 2021-08-24T11:25:23Z

packages/bindings/src/serialport_unix.cpp

@@ -76,7 +76,7 @@ int ToDataBitsConstant(int dataBits) {
  return -1;
 }

-void EIO_Open(napi_env env, void* req) {
+void EIO_Open(void* req) {


env was never used, and I believe it should not be used within the Execute method of the AsyncWorker

Exactly. It must not be used from Execute.

GazHank · 2021-08-24T11:25:32Z

packages/bindings/src/serialport_win.cpp

@@ -56,7 +56,7 @@ void AsyncCloseCallback(uv_handle_t* handle) {
  delete async;
 }

-void EIO_Open(napi_env env, void* req) {
+void EIO_Open(void* req) {


env was never used, and I believe it should not be used within the Execute method of the AsyncWorker

GazHank · 2021-08-24T11:26:27Z

packages/bindings/src/serialport.h

+    EIO_Open(this);
+  }
+
+  void OnError(Napi::Error const &error) override {


Logic from The EIO_AfterOpen method

GazHank · 2021-08-24T11:27:13Z

packages/bindings/src/serialport.cpp

  return env.Undefined();
 }

-void EIO_AfterOpen(napi_env n_env, napi_status status, void* req) {


Logic transfererd to the OnError and OnOK within the struct

GazHank · 2021-08-24T11:37:53Z

Hi @KevinEady & @NickNaso

I've tried to convert OpenBaton to use AsyncWorker per your suggestions, so would be very greatful if you could take a look at those changes; I think it's probably sensible to include any further revisions to that before I roll out similar changes to the other Structs and Methods.

The changes can be found in d042c04

I've added some comments to the commit, but to summarise:
Updated struct definition

Switch AsyncResource to Napi::AsyncWorker
Remove callback and work (provided by AsyncWorker instead)
Transfer EIO_AfterOpen logic (from serialport.cpp) into the OnError and OnOK within the struct (serialport.h)
Override Execute to call EIO_Open (since there is discrete logic for Windows and Linux, I've tried not to touch these, except removing the unused env within the method call)

The changes to the Open method are:

Replace napi_queue_async_work with AsyncWorker->Queue()
Revise how the callback function is passed

This change seems to work based on initial testing, but I've not tried to invoke error conditions, so am not sure if the override of OnError is correct, or the best way to handle things...

reconbot · 2021-08-24T11:42:35Z

AsyncWorker->Queue() this queues the work on the event loop of the context it's getting called in (main loop, worker loop etc), correct?

GazHank · 2021-08-24T18:15:23Z

This method will be called only if you set the error using the method Napi::AsyncWorker::SetError otherwise the Napi::AsyncWorker::OnOK method will be called.
See: https://github.com/nodejs/node-addon-api/blob/main/doc/async_worker.md#seterror and https://github.com/nodejs/node-addon-api/blob/main/doc/async_worker.md#execute
One idea is to call the Napi::AsyncWorker::SetError at the end of the Napi::AsyncWorker::Execute method like repoted below:
  void Execute() override {
    EIO_Open(this);
   SetError(std::string(errorString));
  }

Thanks @NickNaso I assume this is preferable to simply including this logic in the OnOK call?

NickNaso · 2021-08-24T18:20:21Z

This method will be called only if you set the error using the method Napi::AsyncWorker::SetError otherwise the Napi::AsyncWorker::OnOK method will be called.
See: https://github.com/nodejs/node-addon-api/blob/main/doc/async_worker.md#seterror and https://github.com/nodejs/node-addon-api/blob/main/doc/async_worker.md#execute
One idea is to call the Napi::AsyncWorker::SetError at the end of the Napi::AsyncWorker::Execute method like repoted below:
  void Execute() override {
    EIO_Open(this);
   SetError(std::string(errorString));
  }
Thanks @NickNaso I assume this is preferable to simply including this logic in the OnOK call?

Yes you are right.

gabrielschulhof

Looks pretty close 👍

packages/bindings/src/darwin_list.cpp

gabrielschulhof · 2021-08-27T15:36:53Z

packages/bindings/src/serialport.cpp

-
-  uv_queue_work(uv_default_loop(), req, EIO_Update, (uv_after_work_cb)EIO_AfterUpdate);
+  napi_value resource_name;
+  napi_create_string_utf8(env, "Update", NAPI_AUTO_LENGTH, &resource_name);


Suggested change

napi_create_string_utf8(env, "Update", NAPI_AUTO_LENGTH, &resource_name);

NAPI_THROW_IF_FAILED(

napi_create_string_utf8(env, "Update", NAPI_AUTO_LENGTH, &resource_name),

env.Undefined());

Similarly for all other cases where the return value of Node-API calls is being ignored.

Of course, it may be more complicated if a resource has to be torn down before returning, but the pattern is that any and all Node-API calls may fail, and it's a good idea to handle the failure.

gabrielschulhof · 2021-08-27T15:39:31Z

packages/bindings/src/serialport.cpp

-  uv_queue_work(uv_default_loop(), req, EIO_Set, (uv_after_work_cb)EIO_AfterSet);
+
+  napi_value resource_name;
+  napi_create_string_utf8(env, "Set", NAPI_AUTO_LENGTH, &resource_name);


This pattern repeats for all async work. It may be a good idea to factor it out.

gabrielschulhof · 2021-08-27T15:44:51Z

packages/bindings/src/serialport.h

@@ -3,49 +3,48 @@

 // Workaround for electron 11 abi issue https://github.com/serialport/node-serialport/issues/2191
 #include <node_version.h>
-#if CHECK_NODE_MODULE_VERSION && NODE_MODULE_VERSION == 85
+#if CHECK_NODE_API_MODULE_VERSION && NODE_API_MODULE_VERSION == 85


This will tie the addon to the version of V8 it's built against, rendering it non-ABI stable. Perhaps a runtime check of the arguments might be a better approach. Like,

binding.testArgOrder(1, 2);

and then

Napi::Value TestArgOrder(const Napi::CallbackInfo &args) { if (args[0].As<Napi::Number>() == 2) global_js_args_order_reversed = true; }

Thanks @gabrielschulhof I've added this to the TODO list for now, while focussing on broken functionality, but have linked your comment so as to not lose this change.

gabrielschulhof · 2021-08-27T15:46:12Z

packages/bindings/src/serialport_unix.cpp

@@ -76,7 +76,7 @@ int ToDataBitsConstant(int dataBits) {
  return -1;
 }

-void EIO_Open(napi_env env, void* req) {
+void EIO_Open(void* req) {


Exactly. It must not be used from Execute.

GazHank · 2021-09-03T09:43:48Z

Thanks for all of the feedback!

I've migrated all of the common functions to avoid native napi_ calls. The EIO_After functions have been converted to OnError and OnOK within the struct defintions.

Instead of invoking the EIO functions within the Execute() function of the struct, I've directly converted the EIO functions into overrides of the Execute() functions. I think this is cleaner and simpler, but would be keen to get feedback on this (I'd be happy to revert this to the approach previously taken with setBaton if that is preferable).

ps - I was wondering if Napi::AsyncProgressWorker might be a good fit for the read and write functions for windows, but havent had chance to look into that much as yet.

Signed-off-by: Gareth Hancock <[email protected]>

Comment out OnError logic

mac errors, override Execute()

Signed-off-by: Gareth Hancock <[email protected]>

reconbot · 2021-11-26T05:13:33Z

I rebased your branch and pushed it to https://github.com/serialport/node-serialport/compare/GazHank_napi - feel free to reset your branch to that or squash or we can make a pr from that - up to you

GazHank · 2021-11-27T10:32:51Z

Thanks @reconbot

sorry I've been away for a while, hopefully work and travel will calm down a bit as we move towards the end of the year

brandonros · 2021-12-04T19:49:27Z

@GazHank want to rebase and merge? We can cut a beta release from here.

@reconbot could you cut that beta release?

thegnuu · 2021-12-06T10:44:55Z

It would be great to have that beta release soon, it would allow me to implement the changes in our app and make some tests out in the field :)

Eugeny · 2021-12-06T10:45:51Z

Yep, I'll be glad to push it into the Tabby nightly and get feedback.

Migrate from NAN to N-API - this should allow an easier time working with electron and all versions of nodejs * Change dependency to napi * include napi * update shared functionality to NAPI * update unix methods to include napi_env * Convert OpenBaton to AsyncWorker * SetError within Execute() * Replace napi_create_async_work for common methods * remove redundant ByteSize Setting * fix typo * NODE_ADDON_API_ENABLE_MAYBE * optional - remove warnings * Remove OnError logic BREAKING CHANGE: This release switches to NAPI which changes how many binaries are released and will potentially break your build system

reconbot · 2021-12-11T22:25:44Z

This has been released as [email protected] under the beta release tag. (npm i serialport@beta will get it for you.) We're going to stick with beta releases until we get some feedback on the stability. Please try it out!

This is a big day, this has been a major endeavor and a major need of the project for quite a while. I want to thank @GazHank for making it possible. ❤️

GazHank · 2021-12-12T20:10:48Z

I'd like to extend a big thank you to everyone who has supported this; it's been wonderful to see the level of community engagement and contribution!

Eugeny · 2021-12-23T10:28:50Z

Parity options do not work on windows, because the temporary string in ToParityEnum gets destroyed before the comparison, leading to an invalid pointer in str.

Fix:

diff --git a/node_modules/@serialport/bindings/src/serialport.cpp b/node_modules/@serialport/bindings/src/serialport.cpp
index c48e150..00a5f5a 100644
--- a/node_modules/@serialport/bindings/src/serialport.cpp
+++ b/node_modules/@serialport/bindings/src/serialport.cpp
@@ -269,7 +269,8 @@ Napi::Value Drain(const Napi::CallbackInfo& info) {
 }

 inline SerialPortParity ToParityEnum(const Napi::String& napistr) {
-  const char* str = napistr.Utf8Value().c_str();
+  auto tmp = napistr.Utf8Value();
+  const char* str = tmp.c_str();
   size_t count = strlen(str);
   SerialPortParity parity = SERIALPORT_PARITY_NONE;
   if (!strncasecmp(str, "none", count)) {

reconbot · 2021-12-23T15:20:46Z

@Eugeny can you open that in a new issue? I don't want to loose it

GazHank added feature-request Feature or Enhancement help-wanted labels Aug 6, 2021

GazHank added this to the Next milestone Aug 6, 2021

GazHank requested a review from reconbot August 6, 2021 18:33

GazHank mentioned this pull request Aug 6, 2021

N-API support for node-serialport #1186

Closed

reconbot reviewed Aug 19, 2021

View reviewed changes

.generators/package.json Outdated Show resolved Hide resolved

packages/binding-abstract/package.json Outdated Show resolved Hide resolved

KevinEady reviewed Aug 20, 2021

View reviewed changes

NickNaso reviewed Aug 23, 2021

View reviewed changes

packages/bindings/binding.gyp Outdated Show resolved Hide resolved

packages/bindings/src/serialport.cpp Outdated Show resolved Hide resolved

GazHank commented Aug 24, 2021

View reviewed changes

gabrielschulhof reviewed Aug 27, 2021

View reviewed changes

GazHank changed the title ~~Napi migration~~ Node-API migration Sep 3, 2021

GazHank and others added 17 commits November 25, 2021 23:45

optional - remove warnings

82575c3

Signed-off-by: Gareth Hancock <[email protected]>

Remove OnError logic

a1cca06

Comment out OnError logic

Fix linting errors

8383df4

mac errors, override Execute()

linting errors part 2

1a09db0

linting errors part 3

57237f3

package updates

c051b7f

Signed-off-by: Gareth Hancock <[email protected]>

enable exceptions

4bab582

Signed-off-by: Gareth Hancock <[email protected]>

poller fixes

2ee143d

Signed-off-by: Gareth Hancock <[email protected]>

node-abi update

3ab5148

Signed-off-by: Gareth Hancock <[email protected]>

Low Latency fix

540da10

Signed-off-by: Gareth Hancock <[email protected]>

package-lock fix

2bbd67a

Signed-off-by: Gareth Hancock <[email protected]>

Revert loackfile to v1

70a75b2

Signed-off-by: Gareth Hancock <[email protected]>

Cleanup OnError comments

64e8104

Signed-off-by: Gareth Hancock <[email protected]>

Fix Windows Read and Write

42d87d1

Signed-off-by: Gareth Hancock <[email protected]>

Fix no COM port serialport#2325

3950949

Signed-off-by: Gareth Hancock <[email protected]>

fix locks

e0e9b52

fix: locks

e300c78

reconbot force-pushed the napi branch from 1415fcd to e300c78 Compare December 11, 2021 21:57

reconbot merged commit fada6a7 into serialport:master Dec 11, 2021

lovell mentioned this pull request Sep 13, 2023

Builds produced for electron 11 are broken prebuild/prebuild#275

Closed

-  napi_create_string_utf8(env, "Update", NAPI_AUTO_LENGTH, &resource_name);
+  NAPI_THROW_IF_FAILED(
+    napi_create_string_utf8(env, "Update", NAPI_AUTO_LENGTH, &resource_name),
+    env.Undefined());

Node-API migration #2305

Node-API migration #2305

Conversation

GazHank commented Aug 6, 2021 • edited Loading

cinderblock commented Aug 19, 2021

reconbot left a comment

Choose a reason for hiding this comment

reconbot commented Aug 19, 2021 • edited Loading

reconbot commented Aug 19, 2021

cinderblock commented Aug 19, 2021

cinderblock commented Aug 19, 2021

reconbot commented Aug 19, 2021

GazHank commented Aug 19, 2021

cinderblock commented Aug 19, 2021 • edited Loading

KevinEady left a comment • edited Loading

Choose a reason for hiding this comment

GazHank commented Aug 21, 2021 • edited Loading

reconbot commented Aug 21, 2021 • edited Loading

NickNaso left a comment

Choose a reason for hiding this comment

GazHank commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GazHank commented Aug 24, 2021

reconbot commented Aug 24, 2021

GazHank commented Aug 24, 2021

NickNaso commented Aug 24, 2021

gabrielschulhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GazHank commented Sep 3, 2021 • edited Loading

reconbot commented Nov 26, 2021

GazHank commented Nov 27, 2021

brandonros commented Dec 4, 2021 • edited Loading

thegnuu commented Dec 6, 2021

Eugeny commented Dec 6, 2021

reconbot commented Dec 11, 2021

GazHank commented Dec 12, 2021

Eugeny commented Dec 23, 2021

reconbot commented Dec 23, 2021 • edited Loading

GazHank commented Aug 6, 2021 •

edited

Loading

reconbot commented Aug 19, 2021 •

edited

Loading

cinderblock commented Aug 19, 2021 •

edited

Loading

KevinEady left a comment •

edited

Loading

GazHank commented Aug 21, 2021 •

edited

Loading

reconbot commented Aug 21, 2021 •

edited

Loading

GazHank commented Sep 3, 2021 •

edited

Loading

brandonros commented Dec 4, 2021 •

edited

Loading

reconbot commented Dec 23, 2021 •

edited

Loading