Node.js 14 can't build bindings

[boogerlad]

Like this #5667, I'm unable to build the Node.js binding. I'm able to build the project fine though. (osrm-extract, osrm-contract, osrm-routed works fine)

cmake -DENABLE_NODE_BINDINGS=ON ..  
...
make node_osrm

In file included from /root/osrm-backend/include/nodejs/node_osrm.hpp:8,
                 from /root/osrm-backend/src/nodejs/node_osrm.cpp:16:
/root/osrm-backend/node_modules/nan/nan.h: In function ‘void Nan::AsyncQueueWorker(Nan::AsyncWorker*)’:
/root/osrm-backend/node_modules/nan/nan.h:2294:62: error: cast between incompatible function types from ‘void (*)(uv_work_t*)’ {aka ‘void (*)(uv_work_s*)’} to ‘uv_after_work_cb’ {aka ‘void (*)(uv_work_s*, int)’} [-Werror=cast-function-type]
 2294 |     , reinterpret_cast<uv_after_work_cb>(AsyncExecuteComplete)
      |                                                              ^
In file included from /root/osrm-backend/node_modules/nan/nan.h:56,
                 from /root/osrm-backend/include/nodejs/node_osrm.hpp:8,
                 from /root/osrm-backend/src/nodejs/node_osrm.cpp:16:
/root/osrm-backend/include/nodejs/node_osrm.hpp: At global scope:
/root/osrm-backend/build/src/nodejs/node/v12.13.1/include/node.h:566:43: error: cast between incompatible function types from ‘void (*)(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE)’ {aka ‘void (*)(v8::Local<v8::Object>)’} to ‘node::addon_register_func’ {aka ‘void (*)(v8::Local<v8::Object>, v8::Local<v8::Value>, void*)’} [-Werror=cast-function-type]
  566 |       (node::addon_register_func) (regfunc),                          \
      |                                           ^
/root/osrm-backend/build/src/nodejs/node/v12.13.1/include/node.h:600:3: note: in expansion of macro ‘NODE_MODULE_X’
  600 |   NODE_MODULE_X(modname, regfunc, NULL, 0)  // NOLINT (readability/null_usage)
      |   ^~~~~~~~~~~~~
/root/osrm-backend/include/nodejs/node_osrm.hpp:42:1: note: in expansion of macro ‘NODE_MODULE’
   42 | NODE_MODULE(osrm, node_osrm::Engine::Init)
      | ^~~~~~~~~~~

Linux x64, gcc 9.3.0, debian 10. That said, how am I supposed to use this module with my Node.js project? What happens after npm install --build-from-source? How do I reference this custom built module in my project's package.json? Is it intended to be a git submodule?
https://stackoverflow.com/q/30249234

If you do make node_osrm, then the .node file is generated by the usual cmake/make build process. You can also do npm install --build-from-source and in that case, node-gyp is used to build-in-place in the node_modules directory.

[danpat]

You won't be able to use the node bindings with node > 8 until someone puts some work into the bindings layer so it works with the new C++ interface that shipped with node 10+.

[boogerlad]

Thanks! Are the required changes to these files?
https://github.com/Project-OSRM/osrm-backend/blob/master/src/nodejs/node_osrm.cpp
https://github.com/Project-OSRM/osrm-backend/tree/master/include/nodejs

Also, do you know how the performance compares between the HTTP interface and the Node.js bindings? If it's not a big hit, I can live with the HTTP interface.

[danpat]

The performance is a bit dependent on what you want to do. If you're working with large responses with a very large JSON response size (i.e. large /table or routes with very long geometries and lots of annotations), then the C++<->NodeJS bridge can sometimes be a bottleneck.

The HTTP interfaces with osrm-routed is generally pretty fast.

[boogerlad]

Yeah, I'm going to be using the table service exclusively in a one to many configuration, so ~10000 routes each request. Very interesting though. What scenarios is the HTTP interface faster than the Node.js binding? I imagined Node.js binding would be faster in all scenarios. I figured using the HTTP interface internally only would be wasteful, but seems not?

I also thought that with URL length limitations, it would be impossible to pass 10000 points?

[danpat]

The osrm-routed HTTP implementation has no URL length limit - but you'll need to run it with --max-table-size N to allow enough coordinates for your situation, and use a client library that also can support the URL lengths you need - curl works fine up to very large (many megabytes long) URLs.

[boogerlad]

Fantastic! Thanks for the headsup. For anyone else reading this thread: Node.js has a default limit of 8kB

Do you have any benchmarks on Node.js bindings vs HTTP interface?

[danpat]

Do you have any benchmarks on Node.js bindings vs HTTP interface?

Not on hand, but you can perform your own easily enough. I re-implemented most of osrm-routed in Node/Express over in this PR a couple of years ago:

#4604

but we never got around to shipping that. I think it should still work, as long as you use Node 8.

For big requests, using HTTP POST would be an option if you're using Node - you don't have to be bound by the default max header size (although it is also adjustable with a command-line switch).

The big difference between osrm-routed and using Node is safety - nobody has done any kind of security audit on the OSRM HTTP implementation, so while it's done a good job, I'm not sure I'd put my bank details on a system that exposed it raw. osrm-routed is certainly fast enough, but you're on your own WRT security.

There are some known edge cases using Node where things might slow down - particularly requests with many small JSON objects in the response (i.e. large matrices with millions of entries) - the C++<-> NodeJS bridge tends to be a bottleneck here, as all those objects have to be created synchronously on the primary Node thread, which tends to block the event loop.

Over in 0971f06#diff-71b29364088b4fbed3f8557b631291ef I added a renderToJSONBuffer option for the Node module which will cause OSRM to return a pre-serialize Buffer instead of returning an Object - if all you want to do is send the response over the wire, this helps performance enormously for large responses in NodeJS.

[mihneadb]

You won't be able to use the node bindings with node > 8 until someone puts some work into the bindings layer so it works with the new C++ interface that shipped with node 10+.

@danpat is that N-API? Trying to research a bit what it would take to make the project compatible with the latest Node. Thanks

[danpat]

Yes. OSRM is currently using nan.h - https://github.com/nodejs/nan/blob/master/nan.h

I can see that nan.h has been kept up-to-date, but obviously is not backwards compatible. N-API is suppsed to offer that stability, but nobody ever got around to doing the re-write of the bindings for the new stable interface.

[mihneadb]

@danpat I played with this tool a bit and it seems to convert the code accordingly. Unfortunately I don't have knowledge of building binary node packages. I tried playing around with the cmake config in order to get it to include the new napi.h but got into some trouble.

Do you think with your (or some maintainer's) knowledge it might be an easy/quick conversion using that script I linked? Thanks

[danpat]

🤷 I mean, it's probably a couple of hours work to mechanically convert everything over. Finding a couple of hours to do that on the other hand.....

[mihneadb]

@danpat I'm a bit confused about the build step. I see osrm builds the node bindings through the CMake flow. All the N-API tutorials I found online point to using node-gyp. Does this mean I'd have to replace the cmake flow of building the bindings as well?

My issue is not with the mechanical conversion but with setting up the build system.

[danpat]

@mihneadb They can be built either way. If you do make node_osrm, then the .node file is generated by the usual cmake/make build process.

You can also do npm install --build-from-source and in that case, node-gyp is used to build-in-place in the node_modules directory.

I don't think you'd need to modify the build system at all - as long as make node_osrm works, the rest of the setup should already be in place.

[mihneadb]

I needed to manually add an include path for the new napi.h and then I got into more compile errors after applying the script. There are some issues e.g. with namespacing and how the new "register module" functionality works. I don't have enough C++ knowledge & osrm context to get this running, it seems :(.

@danpat hopefully pre-applying that script can cut down on those 2 hours :). This is what the script changed, in case it helps: master...mihneadb:napi

[mihneadb]

@danpat there's a PR up now for node12 at least by improving some uses still of nan. This at least pushes things forward. Let's follow up there - #5732

Thanks!

[bumi001]

@danpat I believe using N-API one should hook into libosrm to reimplement node-osrm. Do you have an opinion?

[systemed]

Fixed in #5918, released with v5.24.