Place1 / node-gir

Node bindings to libgirepository
http://live.gnome.org/GObjectIntrospection
MIT License
27 stars 6 forks source link

Unix specific function #24

Closed clayrisser closed 6 years ago

clayrisser commented 6 years ago

Does anyone know why he following Unix specific function was added to the loop initialization in loop.cpp on line 80? I couldn’t figure it out. I’m assuming fd means file descriptor. It seems to me this will make node-gir incompatible with Windows.

https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#g-source-add-unix-fd

g_source_add_unix_fd(&source->source, uv_backend_fd(loop), (GIOCondition)(G_IO_IN | G_IO_OUT | G_IO_ERR));

It appears it was originally added by Jasper St. Pierre

https://github.com/GNOME/gnode/blob/master/src/loop.cc

Place1 commented 6 years ago

I got the code from here: https://github.com/WebReflection/node-gtk/blob/master/src/loop.cc WebReflection/node-gtk is licenced with MIT.

I'm only just now seeing this GNOME/gnode project. If that is the true souce of the code then we might be in trouble because it's GPL licensed!

As stated, the code I used to implement loop.cpp was from WebReflection/node-gtk and that was done under the impression that that project was the original author and the license was MIT.

We will probably need to re-write this code to ensure license compatability. I'm not sure how to do that re-write in a compatible (i.e. legal) way.

clayrisser commented 6 years ago

I noticed the license too. It seems webreflection copy and pasted the code, then stripped out the license. 🙈

clayrisser commented 6 years ago

If we rewrite it using the same concepts I don’t think we’ll need the license. Personally I think the loop.cpp could be written in a more readable way anyways.

Place1 commented 6 years ago

as for the line of code in question, this like adds the UV event loop's backend file descriptor using uv_backend_fd as an event source for the glib event loop. GLib will poll the source and then run through it's loop lifecycle (source prepare, dispatch, etc) for that source when it detects and event.

In the context of node-gir, this means that once the UV loop has stuff to run (events pending) GLib will be notified and will execute our uv_loop_source_funcs. In the uv_loop_source_dispatch function we uv_run(source->loop, UV_RUN_NOWAIT); which executes the pending events in the UV loop. This is effectively "nesting" the UV loop in the GLib loop.

This is only relevant when you want to run a GLib main loop without totally stopping the nodejs (uv) event loop. In Gtk.js we use this start_loop function to get this nesting happening so that when someone calls gtk.main() it won't block things such as Promises, setTimeout, setInterval, I/O callbacks, etc from executing.

Place1 commented 6 years ago

If we rewrite it using the same concepts I don’t think we’ll need the license. Personally I think the loop.cpp could be written in a more readable way anyways.

Perhaps you can do this rewrite?

clayrisser commented 6 years ago

Yeah, I was planning on doing it personally anyways just so I could understand it better.

clayrisser commented 6 years ago

https://github.com/creationix/node-gir/issues/44

Place1 commented 6 years ago

read my comment in response on that issue. Creationix never used the code.

clayrisser commented 6 years ago

Oh yeah, because node-gir never supported the GTK event loop

clayrisser commented 6 years ago

https://github.com/WebReflection/node-gtk/issues/37

clayrisser commented 6 years ago

Regarding your previous comment, does this prevent the deadlock scenario where the two event loops wait for each other to complete?

I’m guessing the file descriptor is a unix socket. How do you propose to get this to work on Windows? The following is a quote from their documentation.

“As the name suggests, this function is not available on Windows.”

https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#g-source-add-unix-fd

Place1 commented 6 years ago

I suppose we'll need to investigate the alternative for windows and then create a compile time check to choose which implementation to use.

On Sun, Feb 11, 2018 at 2:25 PM, Jam Risser notifications@github.com wrote:

Regarding your previous comment, does this prevent the deadlock scenario where the two event loops wait for each other to complete?

I’m guessing the file descriptor is a unix socket. How do you propose to get this to work on Windows? The following is a quote from their documentation.

“As the name suggests, this function is not available on Windows.”

https://developer.gnome.org/glib/stable/glib-The-Main- Event-Loop.html#g-source-add-unix-fd

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Place1/node-gir/issues/24#issuecomment-364716851, or mute the thread https://github.com/notifications/unsubscribe-auth/ALMBlgEI3ltWySELC2fNhA6mJAuixLGNks5tTl20gaJpZM4SBLKa .

clayrisser commented 6 years ago

Ok, I’ll document this in a more specific issue later

Place1 commented 6 years ago

as for the possibility of a deadlock, I imagine the possibility is no different to running code on UV/GLib loops alone. A dead lock will always occur if code executing on the loop depends on code scheduled to finish in a subsequent loop iteration. There is no problem calling GTK apis from nodejs in this architecture. This architecture is not similar to the early playing around I did in react-native-gtk's bindings which were multi-threaded and flawed as a result.

magcius commented 6 years ago

The way that the mainloop works in gnode / node-gtk is that I effectively stop running the uv mainloop that node starts and run the GLib mainloop instead. The uv mainloop is then pumped through glib's mainloop using the function as described. It's a hack but it worked for the project at the time.

Later on, for a different project, I wrote an updated version of this that uses a thread here, similar to how Electron manages both Chromium's and libuv's event loop.

https://github.com/endlessm/eos-knowledge-content-node/blob/master/src/mainloop.cc

(eos-knowledge-content-node is LGPL, but I'm sure Endless and Matt would be willing to relicense to MIT)

clayrisser commented 6 years ago

We’re safe, it’s licensed under MIT.

https://github.com/WebReflection/node-gtk/issues/37#issuecomment-364717456

https://github.com/WebReflection/node-gtk/commit/3ac6481bfeab82dc522ac799e95b331e628cf1e1

Place1 commented 6 years ago

We’re safe, it’s licensed under MIT.

that's good to hear!

@magcius I'm liking this threaded approach. I'm sure we might want to use something similar in the future!

clayrisser commented 6 years ago

Great explanation @magcius

/*
 * GLibs mainloop is not easy to embed directly right now. So rather then
 * embedding glib's mainloop inside libuvs mainloop, we are running glibs
 * mainloop in a separate thread. However we still want to be able to callback
 * into v8 javascript context from glib async code, so we call the dispatch
 * phase of glibs mainloop back on libuvs main thread. The flow is 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
 * ...
 *
 * If in the future glib moves to epoll to drive its mainloop, we could easily
 * embed it inside of libuvs mainloop on the same thread, by polling on a single
 * fd. See https://bugzilla.gnome.org/show_bug.cgi?id=156048
 */
clayrisser commented 6 years ago

@magcius were you able to MIT license https://github.com/endlessm/eos-knowledge-content-node/blob/master/src/mainloop.cc ?

magcius commented 6 years ago

The copyright for that file would belong to either @mattdangerw or Endless, in which case @ptomato might be able to help relicense it. For what its' worth, I allow any of my contributions to eos-knowledge-content-node to be relicensed under MIT.

ptomato commented 6 years ago

I'll check.

clayrisser commented 6 years ago

Thanks

ptomato commented 6 years ago

Yes, that's OK.

Formally: Endless agrees to relicense the code in https://github.com/endlessm/eos-knowledge-content-node/blob/master/src/mainloop.cc under MIT.

mattdangerw commented 6 years ago

Think endless' OK is what's needed as I was working there when I worked on the multi threaded approach, but in case it's needed--happy to relicense anything in eos-knowledge-content I wrote MIT as well!