upgrade to Node 12 #576

rwalle61 · 2019-05-24T13:45:53Z

(These changes break Node 6. See below)

Fix for Node 12:

General information about the V8 API changes between Node 10 and 12

Summary https://docs.google.com/document/d/1g8JFi8T_oAE_7uAri7Njtig7fKaPDfotU6huOa1alds/edit
"v8/node deprecated APIs and how to handle them" v8/node deprecated APIs and how to handle them bcoin-org/bcrypto#7

These fixes frequently involve the following:

the isolate object - get via Isolate* isolate = v8::Isolate::GetCurrent()
the context object - get via Local<Context> context = Nan::GetCurrentContext()
- https://github.com/nodejs/nan/blob/master/doc/v8_misc.md#nangetcurrentcontext
converting MaybeLocal types returned by many Nan functions to Local using maybeLocalObj.ToLocalChecked()

Specific error fixes and my specific sources of information

from s->WriteUtf8(buf) to s->WriteUtf8(isolate, buf)
- https://github.com/nodejs/node/pull/22531/files#diff-f77b9fed8a3d7f7899ae2316e2bb3230R24
from X->ToString() to Nan::To<String>(X).ToLocalChecked()
- https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29
from X->ToObject() to Nan::To<Object>(X).ToLocalChecked()
- https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29
from X->ToInteger() to Nan::To<Integer>(X).ToLocalChecked()
- following the pattern of the above
- see also https://github.com/nodejs/nan/blob/master/test/cpp/converters.cpp#L37
from X->GetOwnPropertyNames() to Nan::GetOwnPropertyNames(cache).ToLocalChecked()
- https://github.com/nodejs/nan/blob/master/doc/maybe_types.md#api_nan_get_own_property_names
- or, to X->GetOwnPropertyNames(context).ToLocalChecked()
  - https://stackoverflow.com/questions/40533193/v8-c-how-to-get-object-key-values-provided-as-arguments
  - https://v8docs.nodesource.com/node-8.11/db/d85/classv8_1_1_object.html#a79a6e4d66049b9aa648ed4dfdb23e6eb
from X->GetFunction() to X->GetFunction(context).ToLocalChecked()
from String::Utf8Value X(Y) to Nan::Utf8Value X(Y)
from X->Int32Value() to X->Int32Value(context).FromJust()
- https://github.com/microsoft/node-pty/pull/288/files#diff-8fc148e2d2217506b0e485a3033310feR174
from X->IntegerValue() to X->IntegerValue(context).FromJust()
- https://github.com/microsoft/node-pty/pull/288/files#diff-32a8618c58cb849e9b5535ba19ffacc5R186
from kApiPointerSize to kApiSystemPointerSize
- v8/v8@81a654e#diff-4637d1e4b9c1cbab90cbfd580aceff15R29
from isolate->GetCpuProfiler() to ... (larger fix, because whereas the V8 engine previously had just 1 CpuProfiler, it now has multiple)
- https://github.com/nodejs/node/pull/18959/files
- https://v8docs.nodesource.com/node-10.6/d2/d34/classv8_1_1_cpu_profiler.html
from const Handle<String> X to const Local<String> X
- https://electronjs.org/blog/nodejs-native-addons-and-electron-5
- https://github.com/microsoft/node-pty/pull/288/files#diff-8fc148e2d2217506b0e485a3033310feR174
update node-gyp from v3 to v4
- node-gyp rebuild failed on OS X 10.11.1, node -v v4.2.1, node-gyp -v v3.0.3, npm -v 3.3.11 nodejs/node-gyp#792
- tbh I don't know in detail how this solves the problem

Specific deprecation warning fixes and my specific sources of information

from X->Get(Y) to Nan::Get(X, Y).ToLocalChecked()
- https://github.com/nodejs/nan/blob/master/doc/maybe_types.md#nanget
from X->Set(Y, Z) to Nan::SetMethod(X, Y2, Z2)
- (this is for for setting methods. To convert other forms of X->Set(..) to their Nan equivalents, see Nan docs https://github.com/nodejs/nan/blob/master/doc/methods.md#nansetprototypemethod and example https://github.com/astro/node-expat/pull/196/files#diff-2b6f822b34553a6cadd4627d4dc9e84fR29)
- Nan docs https://github.com/nodejs/nan/blob/master/doc/methods.md#api_nan_set_method
- at https://github.com/RuntimeTools/appmetrics/pull/576/files#diff-2419e406d80ff8bfd9b4fc942cc6b491R741, we are using NAN_METHOD macros https://github.com/RuntimeTools/appmetrics/pull/576/files#diff-2419e406d80ff8bfd9b4fc942cc6b491R358

Notes

other forms of 'X->Set' will soon be deprecated - we should update our code

These changes break Node 6:

Problem:

Error output from `npm i` on Node 6

/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:21:10: fatal error: 'utility' file not found
#include <utility>
         ^~~~~~~~~
1 warning and 1 error generated.
make: *** [Release/obj.target/appmetrics/geni/appmetrics.o] Error 1

This can be resolved simply following nodejs/node-gyp#1564 and nodejs/node-gyp#1574. However, that reveals that my upgrade to Node 12 is incompatible with Node 6:

Error output from `CXXFLAGS="-mmacosx-version-min=10.9" LDFLAGS="-mmacosx-version-min=10.9" npm i` on Node 6

In file included from ../src/plugins/node/prof/watchdog.h:21:
../src/plugins/node/prof/compat-inl.h:308:38: error: no member named 'New' in 'v8::CpuProfiler'
    cpu_profiler_ = v8::CpuProfiler::New(isolate);
                    ~~~~~~~~~~~~~~~~~^
Release/obj.target/appmetrics/geni/appmetrics.cpp:467:29: warning: 'Call' is deprecated [-Wdeprecated-declarations]
        listener->callback->Call(argc, argv);
                            ^
../node_modules/nan/nan.h:1673:3: note: 'Call' has been explicitly marked deprecated here
  NAN_DEPRECATED inline v8::Local<v8::Value>
  ^
../node_modules/nan/nan.h:103:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
                                       ^
Release/obj.target/appmetrics/geni/appmetrics.cpp:530:27: error: no matching constructor for initialization of
      'String::Utf8Value'
        String::Utf8Value str(isolate, Nan::To<String>(info[0]).ToLocalChecked());
                          ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable:
      requires single argument 'obj', but 2 arguments were provided
    explicit Utf8Value(Local<v8::Value> obj);
             ^
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable:
      requires 1 argument, but 2 were provided
    Utf8Value(const Utf8Value&);
    ^
Release/obj.target/appmetrics/geni/appmetrics.cpp:542:27: error: no matching constructor for initialization of
      'String::Utf8Value'
        String::Utf8Value str(isolate, Nan::To<String>(info[1]).ToLocalChecked());
                          ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable:
      requires single argument 'obj', but 2 arguments were provided
    explicit Utf8Value(Local<v8::Value> obj);
             ^
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable:
      requires 1 argument, but 2 were provided
    Utf8Value(const Utf8Value&);
    ^
Release/obj.target/appmetrics/geni/appmetrics.cpp:570:27: error: no matching constructor for initialization of
      'String::Utf8Value'
        String::Utf8Value topicArg(isolate, Nan::To<String>(info[0]).ToLocalChecked());
                          ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable:
      requires single argument 'obj', but 2 arguments were provided
    explicit Utf8Value(Local<v8::Value> obj);
             ^
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable:
      requires 1 argument, but 2 were provided
    Utf8Value(const Utf8Value&);
    ^
Release/obj.target/appmetrics/geni/appmetrics.cpp:572:27: error: no matching constructor for initialization of
      'String::Utf8Value'
        String::Utf8Value commandArg(isolate, Nan::To<String>(info[1]).ToLocalChecked());
                          ^          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2428:14: note: candidate constructor not viable:
      requires single argument 'obj', but 2 arguments were provided
    explicit Utf8Value(Local<v8::Value> obj);
             ^
/Users/richard.waller@ibm.com/.node-gyp/6.17.1/include/node/v8.h:2438:5: note: candidate constructor not viable:
      requires 1 argument, but 2 were provided
    Utf8Value(const Utf8Value&);
    ^
1 warning and 5 errors generated.
make: *** [Release/obj.target/appmetrics/geni/appmetrics.o] Error 1

Solution 1

Drop Node 6. This PR achieves this.

Solution 2

Support Node 6 alongside 8, 10, and 12. We would need to spend considerable time converting probably ~60% of the fixes in this PR to use NAN and if blocks.

Ideal long-term solution

Rewrite much of appmetrics using N-API, but that is beyond the scope of this issue.

rwalle61

Commented to explain a couple of lines

src/appmetrics.cpp

rwalle61 · 2019-05-24T14:03:58Z

tests/api_tests.js

-    if (semver.gt(process.version, '10.0.0')) {
+
+    if (semver.gt(process.version, '12.0.0')) {
+      t.skip();


@tobespc suggested skipping this for now

src/appmetrics.cpp

richardlau · 2019-05-24T15:09:16Z

src/appmetrics.cpp

+        s->WriteUtf8(isolate, buf);
+	#else
+        s->WriteUtf8(buf);
+	#endif


Indentation is kind of off here. Typically the preprocessor lines (e.g. those beginning with #) would be in column 0 and the code blocks at the same indentation as if the preprocessor lines were not there.

e.g.

static std::string toStdString(Local<String> s) { char *buf = new char[s->Length() + 1]; #if NODE_VERSION_AT_LEAST(10, 0, 0) Isolate* isolate = v8::Isolate::GetCurrent(); s->WriteUtf8(isolate, buf); #else s->WriteUtf8(buf); #endif

https://github.com/nodejs/nan/blob/master/doc/string_bytes.md#nandecodewrite perhaps? It shouldn't be necessary to have node version ifdefs, at least not for something as common as string conversions.

I'm not sure how to implement that Nan function tbh, as I don't see how the function name and arguments are related. And Nan seems to implement writeUtf8 using 4 or 5 arguments? Happy to implement this if you show me how

src/appmetrics.cpp

AndyRWatson · 2019-05-24T16:02:44Z

I think option 1 is ok given that Node 6 is no longer in LTS. Agree that a longer term plan to support N-Api would be the right one to avoid issues in the future. We should document that Node 6 is no longer supported and the last known working appmetrics level

sam-github · 2019-05-24T17:16:10Z

Dropping 6.x support sounds like a good idea to me, but it requires bumping the appmetrics major (because dropping 6 support is a breaking change for people using node 6). That's NBD, but don't forget to do the book keeping: it eans finding all the other runtimes packages (prometheus, etc) that depend on appmetrics, and publishing an updated major of them, that depends on the new appmetrics major.

sam-github · 2019-05-24T17:19:03Z

src/appmetrics.cpp

+        s->WriteUtf8(isolate, buf);
+	#else
+        s->WriteUtf8(buf);
+	#endif


https://github.com/nodejs/nan/blob/master/doc/string_bytes.md#nandecodewrite perhaps? It shouldn't be necessary to have node version ifdefs, at least not for something as common as string conversions.

sam-github · 2019-05-24T17:23:40Z

src/heapdump/heapdump.cc

@@ -13,7 +13,7 @@
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

 #include "node.h"  // Picks up BUILDING_NODE_EXTENSION on Windows, see #30.
-
+#include "nan.h"


consider grabbing the most recent https://github.com/bnoordhuis/node-heapdump/blob/master/src/heapdump.cc

+1 Richard and I had discussed this, we just need to check we are able to take a new drop.

opened a new issue #579

sam-github · 2019-05-24T17:26:49Z

src/objecttracker.cpp

@@ -37,7 +37,7 @@ NAN_METHOD(getObjectHistogram) {

 	HeapProfiler *heapProfiler = isolate->GetHeapProfiler();

-#if NODE_VERSION_AT_LEAST(4, 0, 0) // > v0.11+
+#if NODE_VERSION_AT_LEAST(4, 0, 0) // > v4.00+


unrelated to this PR, but https://github.com/bnoordhuis/node-heapdump/blob/master/src/heapdump.cc#L69-L78 (which is copied into this repo) has a TakeHeapSnapshot(), maybe it can just be called from heapsnapshot/heapsnapshot.cc?

opened a new issue #579

sam-github · 2019-05-24T17:27:18Z

src/plugins/node/env/nodeenvplugin.cpp

@@ -81,7 +81,13 @@ pushsource* createPushSource(uint32 srcid, const char* name) {

 static std::string ToStdString(Local<String> s) {
 	char *buf = new char[s->Length() + 1];
-	s->WriteUtf8(buf);
+	#if NODE_VERSION_AT_LEAST(10, 0, 0)


see previous comment

Indenting again.

src/plugins/node/prof/nodeprofplugin.cpp

tobespc · 2019-05-28T09:13:23Z

The issues raised in the comments will be dealt with as separate PR's

tobespc

LGTM

src/appmetrics.cpp

hhellyer

I'd like to be sure we need to drop Node 6 support, that feels wrong especially given we work on Node 6 until this goes in.
We need to see whether we can update the heapdump code with the latest from the heapdump npm.

hhellyer · 2019-05-29T12:07:11Z

src/heapdump/heapdump.cc

@@ -13,7 +13,7 @@
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

 #include "node.h"  // Picks up BUILDING_NODE_EXTENSION on Windows, see #30.
-
+#include "nan.h"


+1 Richard and I had discussed this, we just need to check we are able to take a new drop.

src/appmetrics.cpp

rwalle61 · 2019-05-30T10:33:56Z

Travis problem

Build intermittently failing on Windows (various versions of Node) with seemingly the same error in a test in http-outbound-probe-test.js.

Error output:

[Thu May 30 09:47:02 2019] com.ibm.diagnostics.healthcenter.mqtt INFO: Connecting to broker localhost:1883
Error: connect ECONNREFUSED 127.0.0.1:49860
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1191:14)
{ name: 'TAP',
  autoend: true,
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 49860,
  tapCaught: 'uncaughtException',
  test: 'TAP' }
Error: socket hang up
    at createHangUpError (_http_client.js:342:15)
    at Socket.socketCloseListener (_http_client.js:377:23)
    at emitOne (events.js:121:20)
    at Socket.emit (events.js:211:7)
    at TCP._handle.close [as _onclose] (net.js:561:12)
{ name: 'TAP',
  autoend: true,
  code: 'ECONNRESET',
  tapCaught: 'uncaughtException',
  test: 'TAP' }
    # time=92.67ms
not ok 3 - tests/probes/http-outbound-probe-test.js # time=5743.445ms
  ---
  timeout: 120000
  file: tests/probes/http-outbound-probe-test.js
  childId: 2
  command: 'C:\ProgramData\nvs\node\8.16.0\x64\node.exe'
  args:
    - '-r'
    - 'C:\Users\travis\build\RuntimeTools\appmetrics\node_modules\esm\esm.js'
    - tests/probes/http-outbound-probe-test.js
  stdio:
    - 0
    - pipe
    - 2
  cwd: 'C:\Users\travis\build\RuntimeTools\appmetrics'
  exitCode: 1
  ...

Solution

On Travis, Appmetrics passes tests ~60% time on Windows with Node 8, 10, and 12.
We have manually verified that Appmetrics passes tests on Windows with Node 8, 10, and 12.
Travis says "Windows builds are in early access stage"
other people have reported Travis errors due to intermittent network issues https://travis-ci.community/t/nvs-add-6-download-error/3050

Therefore, we will keep running the Windows tests on Travis but flag them as allowed to fail.

That way we will at least be able to monitor whether the Windows tests pass more consistently when Windows builds are no longer just early access.
If the tests still fail intermittently at that point, then we should investigate these failures further

src/appmetrics.cpp

src/objecttracker.cpp

src/plugins/node/env/nodeenvplugin.cpp

src/plugins/node/prof/nodeprofplugin.cpp

rwalle61 · 2019-05-31T09:47:12Z

I've updated the docs, so this PR also resolves #571

hhellyer

Looks good, please use squash and merge to deliver this, there's 20 commits in here and we should add them to master as one logical change.

Richard Waller added 2 commits May 24, 2019 14:18

upgrade to Node 12

b477103

add name to authors.md

e87d7da

rwalle61 requested a review from hhellyer May 24, 2019 13:45

rwalle61 commented May 24, 2019

View reviewed changes

richardlau reviewed May 24, 2019

View reviewed changes

sam-github reviewed May 24, 2019

View reviewed changes

tobespc approved these changes May 28, 2019

View reviewed changes

Richard Waller added 6 commits May 28, 2019 10:22

update travis to run node 12 and stop running 6

7b2b54b

resolve node 12 compilation errors on linux

e83362c

set travis to run on mac instead of linux

39b0868

fix half-fixes that I accidentally pushed

7bae7ce

set travis to test on windows, mac & linux

f8e459d

fix deprecation warnings (obj->get)

2d0abbd

richardlau reviewed May 29, 2019

View reviewed changes

src/appmetrics.cpp Outdated Show resolved Hide resolved

hhellyer requested changes May 29, 2019

View reviewed changes

richardlau reviewed May 29, 2019

View reviewed changes

src/appmetrics.cpp Outdated Show resolved Hide resolved

Richard Waller added 5 commits May 29, 2019 14:40

tidy

0ccff42

fix more Object->Get deprecation warnings

6dc34c1

fix an error on linux

24eee5f

actually fix the linux error

69b5985

fix some Object->Set deprecation warnings

583988e

This was referenced May 29, 2019

Cannot install appmetrics with Node.js v12 on MacOS #572

Closed

Update heapdump.cc #579

Open

Richard Waller added 3 commits May 30, 2019 11:36

remove unused variable

c6913b7

update .travis.yml to allow failures on windows

0191f6d

correct .travis.yml to actually run windows tests

15f55fa

tobespc approved these changes May 30, 2019

View reviewed changes