Introduce a PointerBuffer class for wrapping pointers #64

TimothyJones · 2021-10-04T04:37:01Z

This is the code that @nshmyrev mentioned on #54. It's entirely their code - my only contributions are to merge in the latest upstream changes, confirm the tests pass, and then create this PR.

Note that it introduces a version bump to the next major version. I'm unclear on what the breaking changes are, so I haven't contributed release notes. If needing the release notes will block the merge, then I am happy to read the code and find out.

Once this is merged, I will make a PR for the parent repository, using the fork @nshmyrev mentioned in this comment - #54 (comment)

Please let me know if there is any work that needs to be done before this can be merged.

…build issue

TimothyJones · 2021-10-12T14:06:55Z

@nshmyrev - If you have a moment, would you be able to take a look at the fix for the build failure I made in the last commit?

I'm not very strong with C++ (although I know C well), so I don't think my fix is right. Perhaps the real fix might be just a compiler argument that's missing?

So that you don't have to go digging, the build error was:

..\src\binding.cc(625): error C2248: 'Napi::Env::DefaultFini': cannot access private member declared in class 'Napi::Env' [C:\projects\ref-napi\build\binding.vcxproj]
  c:\projects\ref-napi\node_modules\node-addon-api\napi-inl.h(564): note: see declaration of 'Napi::Env::DefaultFini'
  C:\projects\ref-napi\node_modules\node-addon-api\napi.h(268): note: see declaration of 'Napi::Env'
gyp ERR! build error 
gyp ERR! stack Error: `C:\Program Files (x86)\MSBuild\14.0\bin\MSBuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Program Files (x86)\nodejs\node_modules\npm\node_modules\node-gyp\lib\build.js:194:23)

The failure looks like it's caused by this line in napi.h:

    template <typename T, Finalizer<T> fini = Env::DefaultFini<T>>
    void SetInstanceData(T* data);

However, I couldn't get the failure to happen locally on my mac - so there's something about the CI build that's different. The documented API for SetInstanceData here suggests that the default finaliser is supposed to work if it's invoked without that parameter.

Since the private method it was complaining about just calls delete on the pointer, I worked around it by passing an implementation in directly - but I feel there's probably a more appropriate fix (especially as I'm kind of guessing about the C++ syntax).

TimothyJones · 2021-10-12T14:17:18Z

An aside / question about node 10 for @addaleax - while trying to figure out what was going on with the build, I bumped the version of node-addon-api to the latest, which took us across a major version which drops support for node 10.

The node 10 tests still appear to pass - but since node 10 is EOL (and this PR is a major version bump anyway), do you want to drop node 10 support here too? If you don't want to do that, I can back out the version bump - it wasn't part of the fix.

Either way, I added node 16 to the appveyor build.

TimothyJones · 2021-10-19T06:22:09Z

Is there anything more I can do to help get this moving? It would be very useful for us (and others, I'm sure),

addaleax · 2021-10-19T13:27:51Z

The node 10 tests still appear to pass - but since node 10 is EOL (and this PR is a major version bump anyway), do you want to drop node 10 support here too?

Yeah, it probably makes sense to do this here 👍

Is there anything more I can do to help get this moving? It would be very useful for us (and others, I'm sure),

Yeah, I’m sorry that I don’t have much spare time for review, but it’s at the top of my list :) And I am really grateful for you putting in the work here! 💙

One thing that I would really like to try/do is to use Proxy to enable accessing individual bytes via ptr[index], that would probably take care of the main backwards compatibility concern that I have here.

addaleax · 2021-10-19T13:43:59Z

include/ref-napi.h

@@ -14,7 +14,6 @@ class Instance {
 public:
  virtual napi_value WrapPointer(char* ptr, size_t length) = 0;
  virtual char* GetBufferData(napi_value val) = 0;
-  virtual void RegisterArrayBuffer(napi_value val) = 0;
 };


We can get rid of this file entirely, right?

addaleax · 2021-10-19T13:45:02Z

src/binding.cc

+  Napi::Number value = info[0].As<Napi::Number>();
+  ptr_ = reinterpret_cast<char *>(value.Int64Value());


The JS number type can’t accurately represent 64-bit values, so this will not work as-is, I am afraid :/

addaleax · 2021-10-19T13:46:07Z

src/binding.cc

+     if (encoding == "utf-8") {
+        return String::New(info.Env(), ptr_, length_);
+     } else if (encoding == "ucs2") {
+        return String::New(info.Env(), reinterpret_cast<char16_t*>(ptr_), length_ / 2);


I’d probably just provide a way to read a slice of N bytes starting this pointer as a Buffer (but as a copy, not backed by this address), and then use .toString() on that, so that we keep being compatible with existing code

addaleax · 2021-10-19T13:47:02Z

src/binding.cc

+    throw TypeError::New(env, "readInt64: Cannot read from nullptr pointer");
+  }
+
+  int32_t val = *reinterpret_cast<int32_t*>(ptr);


This will need to be a memcpy();, otherwise it could be an unaligned read

addaleax · 2021-10-19T13:47:55Z

src/binding.cc

+      throw TypeError::New(env, "writeInt32: value out of range");
+  }
+
+  *reinterpret_cast<int32_t*>(ptr) = (int32_t)val;


Ditto here – but I would also really maybe reconsider implementing this in C++, it would be nice to have all the string parsing etc. done in JS and then just write 4 bytes into a temporary Buffer here + provide a method to write a Buffer into a PointerBuffer at a given offset

TimothyJones · 2021-10-29T04:43:38Z

Thanks for the excellent review! I don't think I have the context for the parts that require changes - @nshmyrev, are you able to take a look?

individual-it · 2021-11-17T04:52:38Z

any news here?

mefellows · 2021-11-17T06:30:12Z

I think we're at a bit of an impasse. Tim and (moreso) I don't have the context to make the necessary adjustments (I don't know C++ at all and the pointers from @addaleax might as well be speaking martian to me!) and the maintainers don't seem to have the capacity.

YOU54F · 2022-03-15T02:22:49Z

Hello @addaleax & @TooTallNate ,

First off, incredible work!

We are using this over in https://github.com/pact-foundation/pact-js for contract testing, and love open source!

I am happy to help try and pick up this branch, but I am pretty new to C++

Would you have any spare capacity to help review any further changes, or potentially do a bit of pairing to get this across the line.

Thanks in advance

mefellows · 2022-03-18T10:48:41Z

Thanks Yousaf.

Just an update that we have shifted gears in Pact and are now doing a direct C++ integration layer with node gyp (which seems to be working nicely).

So this is no longer urgent as far as our cause is concerned, but I'm sure future generations of users would appreciate the contribution.

Thanks again for your support to date, whilst we're not likely to use this anymore it got us onto a path of native integration we may never have attempted otherwise.

rturner3 · 2022-04-29T20:44:10Z

@addaleax How much remaining effort is required to get this PR over the finish line? This seems to break users in Node 13-16, and given that Node 17 + 18 are not currently LTS releases, some users can't depend on upgrading to newer versions of Node yet.

I don't have the expertise required to contribute here, but it would be great to know if there is any path forward for this PR.

Refs: node-ffi-napi/ref-napi#64

nshmyrev and others added 7 commits May 7, 2021 00:59

Attempt to use pointer objects instead of buffers

b2d32a8

Fix remaining string tests

7d1a4c8

Bump mocha

d31acc6

Add some missing int reading methods, more needed later

8a9d7b3

Fix argument order

7490921

Properly throw exceptions when value out of range

cad53d2

Merge remote-tracking branch 'upstream/latest' into latest

5f1f184

TimothyJones mentioned this pull request Oct 4, 2021

crash in v8::ArrayBuffer::GetBackingStore #54

Open

mefellows mentioned this pull request Oct 4, 2021

Revert "Feat/ffi consumer" pact-foundation/pact-js-core#335

Merged

TimothyJones added 3 commits October 13, 2021 00:27

Bump version of node-addon-api

34e58cd

Try directly passing the finaliser to SetInstanceData to work around …

609d4f6

…build issue

Add node 16 to appveyor build

6401df0

Remove devDependencies that were also listed in dependencies

aed0caf

TimothyJones mentioned this pull request Oct 13, 2021

Crash in v8::ArrayBuffer::GetBackingStore ffi-cross/js-ffi-cross#9

Open

addaleax reviewed Oct 19, 2021

View reviewed changes

individual-it mentioned this pull request Nov 2, 2021

update pact-js library owncloud/owncloud-sdk#924

Closed

Qantas94Heavy added a commit to Qantas94Heavy/ref-union-di that referenced this pull request Jun 27, 2022

Support PointerBuffer for ref-napi fix

76bae70

Refs: node-ffi-napi/ref-napi#64

angelfraga mentioned this pull request Jan 2, 2024

Error with Electron and ffi-napi node-ffi-napi/node-ffi-napi#238

Open

btsimonh mentioned this pull request Dec 12, 2024

ffi-napi issue on windows while installation openwallet-foundation/credo-ts#2061

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a PointerBuffer class for wrapping pointers #64

Introduce a PointerBuffer class for wrapping pointers #64

TimothyJones commented Oct 4, 2021

TimothyJones commented Oct 12, 2021

TimothyJones commented Oct 12, 2021

TimothyJones commented Oct 19, 2021

addaleax commented Oct 19, 2021

addaleax Oct 19, 2021

addaleax Oct 19, 2021

addaleax Oct 19, 2021

addaleax Oct 19, 2021

addaleax Oct 19, 2021

TimothyJones commented Oct 29, 2021

individual-it commented Nov 17, 2021

mefellows commented Nov 17, 2021

YOU54F commented Mar 15, 2022

mefellows commented Mar 18, 2022

rturner3 commented Apr 29, 2022

		Napi::Number value = info[0].As<Napi::Number>();
		ptr_ = reinterpret_cast<char *>(value.Int64Value());

Introduce a PointerBuffer class for wrapping pointers #64

Are you sure you want to change the base?

Introduce a PointerBuffer class for wrapping pointers #64

Conversation

TimothyJones commented Oct 4, 2021

TimothyJones commented Oct 12, 2021

TimothyJones commented Oct 12, 2021

TimothyJones commented Oct 19, 2021

addaleax commented Oct 19, 2021

addaleax Oct 19, 2021

Choose a reason for hiding this comment

addaleax Oct 19, 2021

Choose a reason for hiding this comment

addaleax Oct 19, 2021

Choose a reason for hiding this comment

addaleax Oct 19, 2021

Choose a reason for hiding this comment

addaleax Oct 19, 2021

Choose a reason for hiding this comment

TimothyJones commented Oct 29, 2021

individual-it commented Nov 17, 2021

mefellows commented Nov 17, 2021

YOU54F commented Mar 15, 2022

mefellows commented Mar 18, 2022

rturner3 commented Apr 29, 2022