-
Notifications
You must be signed in to change notification settings - Fork 695
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
Conversation
Well that test failure is consistently spooky. |
Of course it's not reproducible locally. |
f1b5b7d
to
952878c
Compare
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:
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. |
8fca592
to
4efb675
Compare
Oof... Ok, I think it is our fault after all. I think it's related to: 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. |
4efb675
to
651b315
Compare
119a0ec
to
6a6bc35
Compare
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 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. |
I'm going to take that fix and then split out the thread pool fix into another PR for Context-Awareness |
6a6bc35
to
3206927
Compare
Adds the following unmerged PRs from Libgit2:
core.longpaths
on Windows libgit2/libgit2#5347This 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