T15257 #1

mattdangerw · 2017-03-08T08:06:14Z

Node bindings

https://phabricator.endlessm.com/T15257

We won't need this https://phabricator.endlessm.com/T15257

We are building for node > 6 so we can do this https://phabricator.endlessm.com/T15257

We don't need it https://phabricator.endlessm.com/T15257

ptomato

Nice. The main thing that I think could be improved is returning a promise directly from C++ in the async functions. Otherwise, just a few comments.

I'm having trouble testing it though:

$ nodejs examples/query.js 
module.js:597
  return process.dlopen(module, path._makeLong(filename));
                 ^

Error: Module version mismatch. Expected 48, got 46.
    at Error (native)
    at Object.Module._extensions..node (module.js:597:18)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/philip/checkout/eos-knowledge-content-node/index.js:1:80)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)

I doubt this has anything to do with your code, but I wonder if you've seen this and know what to do about it?

ptomato · 2017-03-08T22:01:22Z

src/engine.cc

+  GObject *engine = GObjectFromWrapper (args[0]);
+  String::Utf8Value id (args[1]->ToString ());
+  String::Utf8Value app_id (args[2]->ToString ());
+  Local<Function> resolve = Local<Function>::Cast(args[3]);


Instead of passing in resolve and reject, I think you can create a persistent v8::Promise::Resolver here, use it directly as the callback data, and return its associated v8::Promise.

ill try that out

ptomato · 2017-03-08T22:06:18Z

src/engine.cc

+  if ((model = eknc_engine_get_object_finish (EKNC_ENGINE (source), result, &error))) {
+    const unsigned argc = 1;
+    Local<Value> argv[argc] = { WrapperFromGObject (isolate, G_OBJECT (model)) };
+    Local<Function>::New(isolate, data->resolve)->Call(isolate->GetCurrentContext()->Global(), argc, argv);


I may be looking at the wrong version of the v8 API docs because I see a different API for this function. Anyhow, is isolate->GetCurrentContext()->Global() being used as the this object for the function call? Does that override any bound this from an arrow function?

Not sure, I think it might. But the functions here are internal to the index.js and not exposed, so I don't think we need to worry about use cases besides the one we have directly.

ptomato · 2017-03-08T22:17:20Z

src/engine.cc

+
+  if ((results = eknc_engine_query_finish (EKNC_ENGINE (source), result, &error))) {
+    const unsigned argc = 1;
+    Local<Value> argv[argc] = { WrapperFromGObject (isolate, G_OBJECT (results)) };


Nothing to do with this commit, but while I was reviewing it I figured it might be good to document that WrapperFromGObject eats a reference

ptomato · 2017-03-08T22:24:21Z

src/value.cc

+            Local<Array> array = Array::New (isolate);
+            GSList *list = (GSList *)arg->v_pointer;
+            for ( ; list != NULL; list = list->next) {
+                array->Set(array->Length(), WrapperFromGObject (isolate, (GObject *)list->data));


It assumes transfer full, exclusively, which may do for now as you say in the commit message. But I think it leaks the GSList, you have to handle GI_TYPE_TAG_GSLIST in FreeGIArgument().

ptomato · 2017-03-08T22:25:24Z

src/value.cc

+        // FIXME: For now, just special case string arrays. Extending to a
+        // general gvariant parser would require the gvariant type in this
+        // function.
+        if (!value->IsArray ())


g_assert (value->IsArray ())?

ptomato · 2017-03-08T23:11:17Z

index.js

@@ -202,6 +201,7 @@ Eknc.Engine.prototype.query = function (query) {
    });
 };

+// Primarily for setting up a 'ekn://' uri handler with electron


Belongs in a different commit?

ptomato · 2017-03-08T23:24:00Z

src/mainloop.cc

+  gint nfds;
+};
+
+static void DispatchGLibMainloop (uv_async_t *async_handle) {


I don't understand too well what's going on here, maybe an overview for dummies?

Sure. Need to update the README too with this from end user perspective.

So this is just for using purely from nodejs (not electron, where chromium uses glib's mainloop to power its event loop called MessageLoop).

The idea is to drive the usual while (running) { poll(fds, timeout); } glib mainloop on a separate thread from libuvs mainloop, because glibs mainloop is not easily embedable. But we still want to be able to have our glib async functions wake up on the main thread so they can calll back into v8s javascript context. So we trigger the specific part of the mainloop that will actually distpach events (g_main_context_dispatch) back on libuv's main thread.

Something like

glib thread: poll for events glib thread: got events. wakeup libuv thread and lock libuv thread: dispatch glib events libuv thread: unlock glib thread glib thread: poll for events ...

Credit to Jasper for the approach.

Long term, if glib moves to epoll (which newer event loops use on linux), we could just poll on a single fd from libuv and drive the glib mainloop in the same thread. Possible with the current state of things, but it would have worked out to a lot more code.

I'll put an abbreviated version of this in the comments.

ptomato · 2017-03-08T23:26:00Z

src/engine.cc

+{
+  Isolate *isolate = Isolate::GetCurrent ();
+  HandleScope scope(isolate);
+  CallbackData *data = (CallbackData *)user_data;


In GJS I've started to use stuff like auto data = static_cast<CallbackData *>(user_data) so that (1) the type name is not specified twice and (2) potentially dangerous casts are made obvious by using C++ style.

I think I'd like to hold off on this for now. There's a lot of cleanups that could be landed, but I'd rather follow the current state of the code brought in from node-gtk for now, which is not using static_cast.

ptomato · 2017-03-08T23:26:58Z

src/bindings.cc

+    const char *prop_name = *prop_name_v;
+
+    GParamSpec *pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (gobject), prop_name);
+    GValue value = { 0, 0 };


Use GValue value = G_VALUE_INIT so you don't depend on the internal structure of GValue

oooh nice ive been looking for that...

ptomato · 2017-03-08T23:27:45Z

index.js


-    return module;
+Eknc.start_glib_mainloop = function () {


Eknc.start_glib_mainloop = bindings.StartGLibMainloop?

mattdangerw · 2017-03-09T20:15:22Z

@ptomato as to everything not working my first suspicion would be node version. I'm on the latest 7.7 right now. Ping me if you want help getting that set up, its not much trouble at all.

Trying to keep this working with a large number of node version is probably doable, but I think best handled as part of https://phabricator.endlessm.com/T15911

We will take a different approach with the mainloop, we need to embed glibs in libuv's and nice vice versa https://phabricator.endlessm.com/T15257

https://phabricator.endlessm.com/T15257

Don't really need it and the name isn't accurate https://phabricator.endlessm.com/T15257

It's really a mix of general gi bindings and content library specific ones at the moment. https://phabricator.endlessm.com/T15257

ptomato · 2017-03-10T02:38:24Z

src/engine.cc

+
+  Local<Promise::Resolver> resolver = Promise::Resolver::New(isolate);
+  Local<Promise> promise = resolver->GetPromise();
+  CallbackData *data = new CallbackData();


Can you just allocate a Persistent<Promise::Resolver> on the heap directly, and get rid of CallbackData?

Totally, but its already in and will scale better to new things we might need. So kinda like keeping the callback data is always a struct approach.

ptomato · 2017-03-10T02:40:47Z

src/engine.cc

+  GError *error = NULL;
+  EkncContentObjectModel *model;
+
+  Local<Promise::Resolver> resolver = Local<Promise::Resolver>::New(isolate, data->resolver);


Why not use data->resolver directly here?

I'll try it. My understanding of the v8 api is pretty damn week. I only got as deep as I needed to get things working. I was trying to mirror the structure of the function code this promise code just replaced, which made a scope, then a local out of the persistent and called into that. But maybe that's not necessary at all?

Yeah, wasn't able to get this working. And most v8 async examples I can find seem to make a local from a persistent and call that.

Oh, I misread it initially, you are creating a new Local<>, not a new Promise::Resolver. Still don't understand why you can't call methods on the Persistent<>, in SpiderMonkey that works fine, but if it doesn't work, it doesn't work...

ptomato · 2017-03-10T02:42:06Z

index.js

+// Setup async functions
+Eknc.Engine.prototype.get_object_for_app = function (id, app_id) {
+    return new Promise((resolve, reject) => {
+        bindings.EngineGetObject(this, id, app_id, resolve, reject);


Not sure how this is working, because the resolve and reject args aren't being read any longer in EngineGetObject(). And IIUC you are just getting a promise returned directly from there, so I don't think you have to wrap it in another promise here.

Oh, just messed up my commit history, one sec...

ptomato · 2017-03-11T00:21:43Z

src/domain.cc

+  if (mime)
+    module_obj->Set (String::NewFromUtf8 (isolate, "mime_type"), String::NewFromUtf8 (isolate, mime));
+  if (bytes)
+    {


Match indentation and bracket styles with the rest of the code

ptomato · 2017-03-11T00:22:16Z

src/domain.cc

+  GError *error = NULL;
+
+  if (!eknc_domain_read_uri (EKNC_DOMAIN (domain), *uri, &bytes, &mime, &error)) {
+    isolate->ThrowException (Exception::TypeError (String::NewFromUtf8 (isolate, error->message)));


TypeError seems like the wrong exception to throw

ptomato · 2017-03-11T00:26:58Z

src/boxed.cc

-static void BoxedClassDestroyed(const WeakCallbackInfo<GIBaseInfo> &cbinfo) {
-    GIBaseInfo *info = cbinfo.GetParameter ();
+static void BoxedClassDestroyed(const WeakCallbackInfo<GIBaseInfo> &data) {
+    GIBaseInfo *info = data.GetParameter ();


This comment got swallowed, I think

ptomato

Only a couple of nitpicks. I would say go ahead and commit it after fixing them, but this needs the other pull request to be merged first anyway.

ptomato · 2017-03-15T21:26:28Z

src/engine.cc

+  else
+    resolver->Reject(Exception::TypeError(String::NewFromUtf8(isolate, error->message)));
+
+  /* XXX: For some reason we have to resolve microtasks to the promise wrappers to resolve. */


Probably can remove this comment, here and below. I've seen this here:

It also seems that it is implied by v8 authors that you should run microtasks manually when you work with promises from C++.

Although I've been googling for a "citation needed" for that and can't find anything.

I'll rework the comment, to say this is expected, but would like to keep something as its pretty confusing.

ptomato · 2017-03-15T21:30:17Z

src/domain.cc

+using namespace v8;
+
+void DomainReadURI(const FunctionCallbackInfo<Value> &args) {
+  Isolate *isolate = args.GetIsolate ();


nitpick: 4 spaces to match rest of codebase

ptomato · 2017-03-15T21:31:34Z

src/mainloop.cc

+using namespace v8;
+
+struct ThreadData {
+  uv_thread_t thread_handle;


nitpick: 4 spaces to match rest of codebase

They will be exposed in node as promises https://phabricator.endlessm.com/T15257

https://phabricator.endlessm.com/T15257

This is fragile code, but it will do for now https://phabricator.endlessm.com/T15257

We will just handle string array gvariants for now, as that's all we really need to get the ekncontent API exposed. Table of contents is a more complicated gvariant type, but for now we'll just dump a verbose string form for industrious javascript code to grok. https://phabricator.endlessm.com/T15257

Chromium ships with a quite recent v8, and electron will run this native module against chromiums javascript interpreter. We need to move of some deprecated v8 functionality. https://phabricator.endlessm.com/T15257

https://phabricator.endlessm.com/T15257

As a node buffer. https://phabricator.endlessm.com/T15257

https://phabricator.endlessm.com/T15257

Turns out this is actually not needed for electron, which will run the glib mainloop as part of its MessageLoop. This is needed for when calling engine async functions directly from node though. https://phabricator.endlessm.com/T15880

https://phabricator.endlessm.com/T15257

ptomato

CR pass

mattdangerw added 3 commits March 8, 2017 00:01

index: remove overrides functionality

e553e26

We won't need this https://phabricator.endlessm.com/T15257

index: use const & let instead of var

207a852

We are building for node > 6 so we can do this https://phabricator.endlessm.com/T15257

index: import ns doesn't take a version

1767502

We don't need it https://phabricator.endlessm.com/T15257

mattdangerw force-pushed the T15257 branch from 4c62f9c to 4779634 Compare March 8, 2017 08:40

ptomato suggested changes Mar 8, 2017

View reviewed changes

mattdangerw force-pushed the T15257 branch from 4779634 to c288c7b Compare March 9, 2017 20:10

mattdangerw added 5 commits March 9, 2017 13:43

gi: remove loop

ed3345e

We will take a different approach with the mainloop, we need to embed glibs in libuv's and nice vice versa https://phabricator.endlessm.com/T15257

function: removed unused function code

5206646

https://phabricator.endlessm.com/T15257

closure: remove closure code

ccc1b17

https://phabricator.endlessm.com/T15257

Remove GNodeJS namespace

dccca44

Don't really need it and the name isn't accurate https://phabricator.endlessm.com/T15257

Rename gi -> bindings

802a21a

It's really a mix of general gi bindings and content library specific ones at the moment. https://phabricator.endlessm.com/T15257

mattdangerw force-pushed the T15257 branch 2 times, most recently from e4081c6 to b086a20 Compare March 10, 2017 00:21

ptomato reviewed Mar 10, 2017

View reviewed changes

mattdangerw force-pushed the T15257 branch from b086a20 to bc817b2 Compare March 10, 2017 18:23

ptomato suggested changes Mar 11, 2017

View reviewed changes

mattdangerw force-pushed the T15257 branch from bc817b2 to d564dbc Compare March 14, 2017 04:26

ptomato suggested changes Mar 15, 2017

View reviewed changes

mattdangerw added 9 commits March 15, 2017 14:48

engine: add bindings for async engine functions

f644c48

They will be exposed in node as promises https://phabricator.endlessm.com/T15257

bindings: really dumb mainloop iteration

d4caa84

https://phabricator.endlessm.com/T15257

value: marshall lists of objects

1d26367

This is fragile code, but it will do for now https://phabricator.endlessm.com/T15257

Update v8 functions

4424044

Chromium ships with a quite recent v8, and electron will run this native module against chromiums javascript interpreter. We need to move of some deprecated v8 functionality. https://phabricator.endlessm.com/T15257

Remove initializer warnings

e2f20c9

https://phabricator.endlessm.com/T15257

domain: add bindings to read the contents of a domain uri

a8ee266

As a node buffer. https://phabricator.endlessm.com/T15257

examples/query: add example script

7960bba

https://phabricator.endlessm.com/T15257

README: fill in with some better documentation

7385315

https://phabricator.endlessm.com/T15257

mattdangerw added 3 commits March 15, 2017 14:49

mainloop: drive the glib mainloop through libuv

7c1010c

Turns out this is actually not needed for electron, which will run the glib mainloop as part of its MessageLoop. This is needed for when calling engine async functions directly from node though. https://phabricator.endlessm.com/T15880

0.0.2

a32d20b

https://phabricator.endlessm.com/T15257

gobject: document that WrapperFromGObject eat a ref

5dca822

https://phabricator.endlessm.com/T15257

mattdangerw force-pushed the T15257 branch from d564dbc to 5dca822 Compare March 15, 2017 21:49

ptomato approved these changes Mar 15, 2017

View reviewed changes

ptomato merged commit f282aa5 into master Mar 15, 2017

ptomato deleted the T15257 branch March 15, 2017 22:13

T15257 #1

T15257 #1

Conversation

mattdangerw commented Mar 8, 2017

ptomato 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

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

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

mattdangerw commented Mar 9, 2017

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

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

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdangerw Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato left a comment

Choose a reason for hiding this comment

mattdangerw Mar 15, 2017 •

edited

Loading