Reduce and isolate usage of Dir.chdir #383
Conversation
Instead of using chdir blocks, we should use absolute paths so that we can enable threaded usage of the library If Process.fork() is available, we'll use that for git.chdir actions, otherwise, we'll use a global mutex to prevent threads from calling git.chdir before the previous execution has completed. - fork() when using Dir.chdir, otherwise, use a mutex - wrap calls to in a semaphore - replace Dir.chdir with paths - fixes #355 Note: if you do async work in git.chdir all bets are off. Signed-off-by: Joshua Liebowitz <taquitos@google.com>
|
@perlun, this one should be good to go as soon as CI is done. I squashed all my commits into 1. |
| else | ||
| do_get.call | ||
| if @git_dir | ||
| Dir.chdir(@git_dir, &do_get) |
There was a problem hiding this comment.
This part is still non-threadsafe - was this intentional?
| else | ||
| build_list.call | ||
| if @git_dir | ||
| Dir.chdir(@git_dir, &build_list) |
There was a problem hiding this comment.
Yeah, I thought I took care of these, might just be a merge issue. I'm offline for a week now, but will take a look.
|
I am interested, how this will work inside a Sidekiq worker, which is known for it's inability to fork processes sidekiq/sidekiq#783 (comment) Perhaps whether we use fork vs mutex can also be a customizable option |
|
@taquitos Any updates on this? It would be nice to get it merged, sooner or later. |
|
Unfortunately I won't have time to continue this PR. I've joined a new team and am no longer working on Ruby things, and my free time is pretty limited these days. Of course, anybody should feel free to continue to pull this over the finish line. |
Fair enough - it's unfortunately quite the same for me, I've switched jobs and is no longer working on Ruby things either. 😊 So... unless someone (@tarcinil , @mib32) is willing to pick up the gauntlet on this one, I suspect it will be auto-closed in a few months, regretfully. I think it was a really great initiative since thread safety is not an option; it should be the default in all code we write, and non-thread-safe is really "opt in" for extreme circumstances. Regretfully, this is not really the case with our code base as it is so if people feel like volunteering to fix it - I am more than happy to see it coming. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing due to lack of interest. If anyone wants to take this up, I am happy to help with review. This also needs a test. |
Instead of using chdir blocks, we should use absolute paths so that we can enable threaded usage of the library
If Process.fork() is available, we'll use that for git.chdir actions, otherwise, we'll use a global mutex to
prevent threads from calling git.chdir before the previous execution has completed.
Note: if you do async work in git.chdir all bets are off.
Your checklist for this pull request
🚨Please review the guidelines for contributing to this repository.
Signed-off-by: Joshua Liebowitz taquitos@google.com