Skip to content

Bump libgit2 to fork of v1.0.0 #1788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 25, 2020

Conversation

implausible
Copy link
Member

@implausible implausible commented Aug 13, 2020

Adds the following unmerged PRs from Libgit2:

This should lay the groundwork for having correct async resources in the context aware version of NodeGit thanks to the custom tls implementation added to libgit2.

This PR is necessary to move #1774 forward more

@implausible
Copy link
Member Author

Well that test failure is consistently spooky.

@implausible
Copy link
Member Author

Of course it's not reproducible locally.

@implausible implausible force-pushed the bump/libgit2-1.0.0 branch 12 times, most recently from f1b5b7d to 952878c Compare August 14, 2020 21:21
@implausible
Copy link
Member Author

Holy hell that is gnarly, but I don't think it's us, I think something is wrong with mocha on Node.

Here's what happens:

  1. RunLoopCallbacks is scheduled on the event loop
  2. credentials_async is called
  3. ForwardIfPromise is called, it detects that it is a promise, mounts the callbacks on then, and exits with true
  4. credentials_async exits
  5. RunLoopCallbacks exits
  6. The promise chain never starts.
  7. Mocha cancels the test
  8. The promise chain starts
  9. The promise chain finishes
  10. credentials_promiseCompleted is called and finishes

The only way I think this could happen is if mocha had shceduled something that is scheduled to run before the promise chain built in the credentials callback, and then it blocks the javascript thread until mocha cancels it. (mocha must be in control here, because mocha is able to cancel the test).

Fucking wild.

I've temporarily disabled that portion of the test... I am unable to place how this would be NodeGit's fault.

@implausible implausible force-pushed the bump/libgit2-1.0.0 branch 5 times, most recently from 8fca592 to 4efb675 Compare August 17, 2020 17:49
@implausible
Copy link
Member Author

Oof... Ok, I think it is our fault after all. I think it's related to:
nodejs/node#5691

I think we need to fix async resources to solve this issue, which means that this bump to 1.0.0 can't just be a bump to 1.0.0, it needs to also fix #1774. I was hoping to bring in the patches we need in libgit2 into NodeGit before fixing async resources, but with this issue being so fucking consistent, I think there's no other way.

Also, I noticed that this behavior can occur outside of node 14 x64 on windows, I've seen this exact behavior tank the rebase commit signing callback tests on Node 10. I think this might be a case of undefined behavior and a game of wacky wavy inflatable timeouts.

All I know is the second that timeout callback occurs, it allows the frozen promise chain to start running.

@implausible implausible force-pushed the bump/libgit2-1.0.0 branch 3 times, most recently from 119a0ec to 6a6bc35 Compare August 25, 2020 20:45
@implausible
Copy link
Member Author

Ok after a pretty intense refactor of the ThreadPool to support AsyncResource passing, I discovered that it's not at fault. I still needed to do the refactor to get AsyncResource passing as part of the Context Awareness refactor, so that's fine. What it all boiled down to was that we need to initialize a node::CallbackScope when we enter the libuv event loop. Without running this, all of the things that node does in the background after coming back from a thread aren't scheduled to run. In a bizarre circumstance, this always triggered on Windows 10 x64 Node 14.

This issue roughly explains the problem nodejs/node#5691. I don't know why we don't run into this more often, but it's definitely not a problem in nodegit anymore. Wild.

@implausible
Copy link
Member Author

I'm going to take that fix and then split out the thread pool fix into another PR for Context-Awareness

@implausible implausible merged commit 02e617b into nodegit:master Aug 25, 2020
@implausible implausible deleted the bump/libgit2-1.0.0 branch August 25, 2020 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant