Make nodegit context-aware for compatibility with Electron 9 and beyond

Electron 9 changed the default value of app.allowRendererProcessReuse to true, which breaks nodegit. Setting it to false manually fixes the issue, but this will only be possible until Electron 11.

Are there any plans to fix this? Seems like #1298 might be related. Also see electron/electron#18397 for more information.

System information

node version: v12.16.3
npm or yarn version: 6.14.4
OS/version/architecture: win32 10.0.18363 x64
Applicable nodegit version: 0.26.5

node -v
npm -v # (or yarn -v)
node -e "console.log(process.platform)"
node -e "console.log(require('os').release())"
node -e "console.log(console.log(process.arch))"

This definitely needs to be looked in to, if not prioritized, for the future of nodegit in Electron environments given how fast Electron releases are rolling out. With RC for Electron 10 already out, it's only a matter of (short) time before 11 is the norm.

Is there a preference for context-awareness over n-api compat?

Axosoft is aware that we need to update this shortly. We'll be putting 2 engineers on this after our next GitKraken release. We want to hit this before Electron 11 is in beta. GitKraken essentially needs this to happen no matter what, so it will be prioritized.

As for general context-awareness vs n-api compat, definitely prefer switching to n-api compat, that seems like a better long term. There's also a ton of utility in n-api that should simplify a lot of extra work we've had to do (thread safe callbacks, I'm looking at you).

Sit tight, it will arrive in time for Electron 11.

@maxkorp

Awesome, good to hear that!

@maxkorp To be honest I have no idea what the difference and generally the context here is, I just used the terminology I saw in the warning messages :p

An update: I'm starting work on this officially. I am currently working on making sure that I'm not going to back myself into a corner by moving to N-API. There are some concerns I have with moving toward N-API that have to deal with the differences between ObjectWrap in N-API and in NAN and how we use them in NodeGit. In particular the inheritance model for https://github.com/nodegit/nodegit/blob/master/generate/templates/manual/include/nodegit_wrapper.h has me a bit worried about initializing classes using https://github.com/nodejs/node-addon-api/blob/master/doc/object_wrap.md. It almost seems incompatible to me because of the DefineClass macro. This is possibly my own shortcoming that I don't necessarily know how to make these compatible with each other...

The other concerning thing is to do with our use of an internal thread pool. We built a thread pool #1019 here to solve thread pool saturation when sharing with the standard node pool; if we moved to N-API i suppose that this would mean that users would need to consider increasing UV_THREADPOOL_SIZE to prevent saturation... It also could be resolved by having NodeGit itself run in a worker thread (a benefit of context-aware native modules)... but that would actually be a holistic nightmare for devs of GitKraken and I assume current users of nodegit for the amount of refactor necessary to move nodegit off of the main loop and onto a worker...

It seems like between some of those problems, Context Aware API is probably not only a shorter change, but less destructive downstream. I'm not a fan of NAN and raw v8, and NAPI is a lot cleaner imo, but we're not even going to get the benefits of ABI compatibility because of our reliance on libuv for a separate thread pool and the OpenSSL symbols that ship with Node.

All of this is to say, we're still going to be context aware by the time Electron 11 lands, but I think I'm going to change direction and try to update the library to be context-aware instead.

I think some of my major goals here are:

Fix async resources across async workers and callbacks
Ensure worker threads are 100% supported by the library.

After doing some reading up, I think that's a fair evaluation and a good direction to head

Making some progress with modeling the threadpool updates. Ran into a snag with Node's cleanup callback. Looks to be solved here nodejs/node#34567. I'm going to continue development with the thread pool with the assumption that this will land downstream in Node sometime soon. If that's in betore I finish great! If not, then I will end up shutting off worker support until it does land.

Hm, this opens up the door for the next real issue, since a libgit2 execute thread can be active during shutdown, that libgit2 function could be trying to make callbacks back into javascript. In those cases, we need to be sending back the cancellation flag back to libgit2 on any callbacks that the user has defined. For callbacks that don't have cancellation, we just need to ignore them. I think in general, we want the libgit2 functions to either finish to completion or cancel when they require more javascript interation.

This is all predicated on what the Node devs have explained, which is that once cleanup of a worker thread has started, no more attempts should be made to communicate back to the javascript thread. At that point we should wait for completion of all execute threads while actively cancelling operations via callbacks when we see them come through. Any callback batons that get created should forcefully close if they do not have results ready from the JS thread.

That was a wild ride, but we think we got worker threads and async resources fully functional in #1795

@implausible

@implausible Awesome! Great work guys :) Looking forward to a release!

I'm not sure what the timeline is atm. We're waiting on a backport of async native node module cleanup for node 12. It looks like we're going to have to drop support for Node 10, and Electron 8/9 as no backport was mentioned for it.

@implausible

@implausible Is there an issue/place to track the progress of the backport for node?

So we merged this today, you can expect we'll get an alpha going shortly that will give an opportunity for users to test the updated NodeGit for stability. It looks like the required patches to run NodeGit for Electron will not make it into 9-x-y, nor will Node 10 be pulling in the updates as far as I can tell (though they might at a later date).

For now after we release the alpha, we will have new minimum targets for Electron and Node.
Node: > 12.19 and > 14.10.0
Electron: > 10.1.4 and > 11.0.0-beta.8

I suppose we'll keep it in alpha until Electron 11.0.0 is released or until we see support land in Electron 9-x-y and Node 10.

@implausible

Hey @implausible - had some time so went to test this today, but noticed the alpha isn't published on NPM

Also, Electron 11 has since been released - have there been enough tests to publish 0.28.0? If not, could the alpha be published on NPM?

Thanks

If not, could the alpha be published on NPM?

I was working on this last week and continue to work on it this week. Something has gone wrong in CI since the last publish. I've fixed the nodegit repo's side of things, but the electron prebuilder job is broken for mac and windows. Once that is fixed the alpha will be on NPM.

Oof it looks like the API provided by the node team was not ABI compatible. It looks like chromium in M75 started using a different std lib. We'll have to get that patched in Node and Electron. In the meantime it looks like NAPI might provide a quick workaround... Working on it.

I'm reopening this issue as there is no workaround without attempting to change compilers for NodeGit entirely. It looks like we're going to need to wait for a patch from Node and then Electron I've opened an issue surrounding what's stopping us from compiling and introducing our context-awareness changes with Node. Apologies to anyone who was affected by this delay. We should have at least checked on the beta build with Electron 11, but I had not anticipated such a dramatic change to how Electron is compiled.

nodejs/node#36349

We'll be picking this up again. Thanks for everyone's patience. It looks like we're heading down the rabbit hole of getting node-gyp to play nicely with clang on Windows via VS2017.

I think we'll need to open up some PRs to https://github.com/nodejs/node-gyp and https://github.com/felixrieseberg/windows-build-tools so that we can normalize the installation of clang on windows as well as force Electron compilation to leverage clang on Electron 11 and above.

I think once we're there we can get this built and moved out.

Heads up to those who need this. I am currently super locked at work with a lot of high priority tasks and this is slipping. I have made progress and have a potential fix, but it needs to be merged into Electron and a release needs to be made before we can move forward here.

This patch needs to make it into 11-x-y and 12-x-y on Electron nodejs/node#37000. Once this is in, I have tested and confirmed that NodeGit will compile for Electron.

It's not likely I will open this before end of month, but I will try to get it done by end of month. The GitKraken team needs this, so it won't be dropped at the minimum.

Again sorry for the delays.

For those following, I finally have good news. The end is in sight. electron/electron#28043 when this issue has been backported/released we can release for Electron 11.

Updating, after these 3 merge: electron/electron#28108, electron/electron#28109, electron/electron#28110

Those are merged now, next up is Electron to release their latest builds

Any progress?

I think this issue can be closed now?

@MikeJerred

@MikeJerred Is it solved then? I don't see any new releases confirming this ...

@implausible

No, but the comments by @implausible seem to suggest that this issue would be fixed once certain PRs get merged to electron and nodejs - all of which have since been merged. Also I am able to build and run it fine on modern versions of electron - I have tried v12.0.7 and v16.0.4 so far which shouldn't be possible according if this issue were outstanding.

Good to hear, but I rather wait for the official release to mark it solved, than rely on alpha releases...

Navigation Menu

Repository navigation

Make nodegit context-aware for compatibility with Electron 9 and beyond #1774

System information

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Make nodegit context-aware for compatibility with Electron 9 and beyond #1774

Description

System information

Activity

devlsh commented on Jun 6, 2020

maxkorp commented on Jun 9, 2020

implausible commented on Jun 9, 2020

Livven commented on Jun 9, 2020

implausible commented on Jul 30, 2020

maxkorp commented on Jul 31, 2020

implausible commented on Jul 31, 2020

implausible commented on Jul 31, 2020

implausible commented on Sep 11, 2020

devlsh commented on Sep 21, 2020

implausible commented on Sep 21, 2020

devlsh commented on Oct 16, 2020

implausible commented on Oct 20, 2020

devlsh commented on Nov 29, 2020

implausible commented on Nov 30, 2020

implausible commented on Dec 1, 2020

implausible commented on Dec 1, 2020

implausible commented on Jan 4, 2021

implausible commented on Feb 5, 2021

implausible commented on Mar 9, 2021

implausible commented on Mar 12, 2021

implausible commented on Mar 15, 2021

ylc395 commented on May 28, 2021

MikeJerred commented on Dec 16, 2021

gpetrov commented on Dec 17, 2021

MikeJerred commented on Dec 17, 2021

gpetrov commented on Dec 17, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions