-
Notifications
You must be signed in to change notification settings - Fork 899
Request for Asynchronous Support (Async/Await)? #1440
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 se 8000 rvice and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
You could wrap the commands in Task.Run() |
@solvingj there's a rather large meta-debate about if libraries should force asynchronous methodologies on consumers. Given that a consumer of a library could have specialized scheduler, thread affinity, etc. needs, doing so can prevent many would be consumers from ever consuming the library. Theorycraft aside, Libgit2 (the real library, the C# layer is just a wrapper) isn't really thread safe - so it would likely be a bad idea to encourage multi-threaded usage of the library. |
@whoisj that is interesting, thanks so much for the feedback. If the underlying layer isn't threadsafe and Async capable, then it seems there's nothing more to be done about it. As for the meta-debate, from my point of view as a consumer, it seems reasonable in those specialized cases to have to adapt to library code by appending async method calls with GetAwaiter().GetResult() to make them run effectively synchronous. Conversely, if a library doesn't expose Async methods... there's no way to "convert" them the other way. However, as with any large-scale debates, I'm willing to believe that there might be many factors outside my personal experience and perspective which complicate the issue to the point where such armchair assessments aren't valid. I'm sure there are lots of smart people who have found real issues on both sides of the coin. It's great to learn about these things so I can go research later, and maybe someday have an educated opinion. Thanks for the tip! |
Can we reopen this? Async/await is not just about multi-threading, it's about being able to do networking operations in a non-blocking fashion. If one is running a service that has to do quite a few of those the service can easily choke currently. Could we reopen this @solvingj ? |
My understanding is there anything done in this library with async/await would ultimately create an unsafe situation because the C library itself is not thread safe. If someone can find where the C library has advertised that it now thread safe, then yes. If not, then I do not think we should reopen it. It's been seven years, it seems plausible that they may have made such a change. |
What does "thread safe" mean? Some parts of libgit2 are "thread safe" in that you can mutate parts of the object database (for example) from different threads. Some parts are not, meaning that you cannot mutate a repository in two different threads.
|
I think just offering async/await for non-blocking interactions with the remote would be very useful for people using this library on a server. If not async await, then just some other way to do non-blocking IO. |
The problem is that since everything is going through libgit2, there is no real way to do non-blocking I/O. The best you could do is what @ethomson mentions, which is have a dedicated worker thread, but that is pretty much an anti-pattern for async code paths. You'd still be blocking that worker thread for the calls into libgit2. |
That makes sense but I found this PR in Would it be possible to surface some of that in |
I'm honestly not sure. It would entirely depend on how that is exposed and accessible from the libgit2 APIs and if there is a way to hook that into some sort of async mechanisms on the managed side. @ethomson What do the APIs for that look like? |
Hi @ethomson, do you reckon this is feasible to expose to C#? It would be pretty helpful for using |
I could be wrong, but it's my understanding that all the operations are synchronous in nature and therefor blocking. Is there any possibility that this library will support Asynchronous use?
The text was updated successfully, but these errors were encountered: