Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Move libuv calls to correct thread#1197

Merged
johnhaley81 merged 5 commits intonodegit:masternodegit/nodegit:masterfrom
srajko:uv_close-fixsrajko/nodegit:uv_close-fixCopy head branch name to clipboard
Feb 6, 2017
Merged

Move libuv calls to correct thread#1197
johnhaley81 merged 5 commits intonodegit:masternodegit/nodegit:masterfrom
srajko:uv_close-fixsrajko/nodegit:uv_close-fixCopy head branch name to clipboard

Conversation

@srajko
Copy link
Collaborator

@srajko srajko commented Jan 27, 2017

This PR removes uv_async_init / uv_close calls that were being called each time we were making callbacks from async threadpool threads into the main thread (for example, for clone progress callbacks). The change was prompted by Axosoft/nsfw#12 - in our case, the uv_async_init calls were being called from threadpool threads and that violated libuv's thread safety requirements.

The fix was to move the logic for these types of callbacks (I call them reverse callbacks in the code, since they are callbacks from the async threadpool code back into the main loop code) into our ThreadPool class, which now uses a single async handle and an internal queue to schedule these callbacks.

While at it, I also removed our last use of sleep_for_ms and replaced it with a semaphore.

We also had a return that would circumvent the uv_close -

. With this change, that problem goes away.

Finally, the batons we were using for these reverse callbacks were dynamically allocated and never deallocated. I took out the dynamic allocation to avoid this problem.

@srajko srajko changed the title Move uv_close calls to main thread [WIP] Move libuv calls to correct thread Jan 31, 2017
@srajko srajko changed the title [WIP] Move libuv calls to correct thread Move libuv calls to correct thread Feb 6, 2017
@srajko
Copy link
Collaborator Author

srajko commented Feb 6, 2017

This was tested for stability in a staging GitKraken build, we didn't run into any problems.

@johnhaley81
Copy link
Collaborator

💯 Memory leak fixes and stability increases. 💯

@srajko talked offline about some naming issues that the PR brought out to the front. Namely that ExecuteAsync was very poorly named and lead to some confusion since it's not really "async" like JS async where you schedule a callback and return out of a function. ExecuteAsync schedules a callback and then in C++ land uses a semaphore to wait for a promise to resolve if the returned value was a promise.

We both decided that we can update the names of the underlying callback system in a different PR and that this is GTG.

@johnhaley81 johnhaley81 merged commit 3828c83 into nodegit:master Feb 6, 2017
@johnhaley81 johnhaley81 deleted the uv_close-fix branch February 6, 2017 16:24
tommoor pushed a commit to goabstract/nodegit that referenced this pull request Mar 27, 2017
Move libuv calls to correct thread
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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.