Skip to content

Conversation

@implausible
Copy link
Contributor

In Node-land, it is super important that we are able to track context across async operations for things like async stack traces and for ensuring that we are attached to the correct v8 context and isolate. One particular pattern for ensuring that a library is leveraging the correct context across a thread pool is by managing thread local storage to track the context as we builds threads.

In LibGit2, because it internally spins up its own threads, it is not possible or easy to build TLS for the internal threads. This means that certain operations that have internal threading in libgit2 (pack building or hopefully someday checkout!), if any callbacks make their way from those internal threads in libgit2, those will not have their TLS initialized to the same context of the thread that requested the libgit2 operation.

Short of providing the ability to do a drop-in replacement for the threading library like libgit2 does allocation, this PR introduces a series of callbacks that will enable a user of the libgit2 library to manage some TLS for internal libgit2 threads for context tracking.

The general flow is for a libgit2 operation to run a callback before initializing a thread internally to provide a calling library an opportunity to seed some TLS on the internally. After a thread is spun up in libgit2, libgit2 will send that pointer back to the user of libgit2 for them to store/set their TLS. Finally, when a thread is exiting in libgit2, we will call a cleanup callback to allow a user of the library to clean up any associated handles.

I've thought long and hard about what's the least disruptive way to allow for this in the library, and this seems like a nice way forward.

@implausible implausible force-pushed the feature/custom-tls-for-external-library branch 2 times, most recently from b34cf3c to 4d1062d Compare August 6, 2020 15:17
@ethomson
Copy link
Member

Sorry to take so long to review this. I think that this is a good direction, but I think that I'd do a little refactoring on the nomenclature. If I'm reading this correctly, these are really hooks into the thread creation and destruction functions for callers. And so users could use this to configure TLS data, that's presumably not the only thing that they could do with it. I suppose that they could attach some debugging or instrumentation, or ... I'm not sure. 😀

Maybe something like git_thread_set_callbacks with an init and a shutdown callback, where the init callback can return an arbitrary data and that will be given back to the shutdown callback? I think that's basically what you have here, just with some naming changes to make it more generic?

@implausible
Copy link
Contributor Author

Ah yes, that makes sense. I will rename these methods and structs to mirror thread lifetimes instead.

Base automatically changed from master to main January 7, 2021 10:09
@implausible implausible force-pushed the feature/custom-tls-for-external-library branch from 4d1062d to 1bf8564 Compare March 29, 2021 18:02
@implausible implausible force-pushed the feature/custom-tls-for-external-library branch from 1bf8564 to 32e810e Compare March 29, 2021 20:04
@implausible
Copy link
Contributor Author

I know I am moving this branch around, but I have not implemented the changes yet. Just need to move this up higher today to fix an issue in NodeGit.

Plans for getting this branch into libgit2:

  • Move all of custom_tls into threadstate
  • Rename git_custom_tls_set_callbacks to git_thread_set_callbacks

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