Isolate Dir.chdir to a new process, or mutex #372

taquitos · 2018-06-25T18:11:01Z

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.

Fixes #355

Note: if you do async work in git.chdir all bets are off.

Signed-off-by: Joshua Liebowitz

Ensure all commits include DCO sign-off.
Ensure that your contributions pass unit testing.
Ensure that your contributions contain documentation if applicable.

Instead of using chdir blocks, we should use absolute paths so that we can enable threaded usage of the library Fixes #355

taquitos · 2018-06-25T18:12:29Z

lib/git/lib.rb

-      if @git_dir
-        Dir.chdir(@git_dir, &do_get)
-      else
-        build_list.call


this is a typo in the original code, submitting a pr for this

perlun · 2018-06-25T19:21:47Z

@taquitos Much appreciated! 👏 Let me know once you have this under control.

Once this is sorted out, we'll probably get #358 fixed for free.

Signed-off-by: Joshua Liebowitz <taquitos@google.com>

taquitos · 2018-06-25T21:48:28Z

I am now testing with the latest change. Will update after we have it running for a bit.

355

Now that we’ve made progress on ruby-git/ruby-git#372 We can limit mutexes to be individual repo-scoped instead of 1 giant mutex for all.

Signed-off-by: Joshua Liebowitz <taquitos@google.com>

taquitos · 2018-06-25T22:33:13Z

lib/git/lib.rb

-      else
-        do_get.call
-      end
+      do_get.call(@git_dir)


not 💯 this will work, but tests pass. Seems like we might need to pass the path to the config command.

taquitos · 2018-06-25T22:33:48Z

lib/git/lib.rb

-      else
-        build_list.call
-      end
+      build_list.call(@git_dir)


same here, I think we need to utilize the path variable in the config command if it exists

Signed-off-by: Joshua Liebowitz <taquitos@google.com>

taquitos · 2018-06-25T23:25:42Z

lib/git/lib.rb

@@ -8,6 +8,7 @@ class GitExecuteError < StandardError
  class Lib

    @@semaphore = Mutex.new
+    @@config_semaphore = Mutex.new


calls to config also should be protected, but it might be heavy handed to fork

taquitos · 2018-06-25T23:30:54Z

@perlun I'd love to get your thoughts. The changes don't make everything thread-safe, but it takes care of a lot of issues. There are a few places where paths can't be passed in $git config is one of those, so we must Dir.chdir 🙄when we're dealing with local config.

355

perlun · 2018-07-10T09:39:21Z

@taquitos Thanks, will try to get this reviewed ASAP. Two minor things:

Could you rebase/merge in master into the branch?
Could you add the DCO?

Thanks in advance.

taquitos · 2018-07-17T18:02:01Z

@perlun when you get a moment 👍

perlun

Some very good fixes here, bravo! 👏

I gave some notes on things I think could be addressed before merge. Please let me know what you think about these.

perlun · 2018-07-17T18:59:29Z

lib/git/base.rb

+    class << self
+      attr_accessor :chdir_semaphore
+    end
+    Git::Base.chdir_semaphore = Mutex.new


This works perfectly fine, but maybe the approach below would be even better. It shouldn't really be an accessor but more like a reader since it's an immutable data structure that shouldn't be tampered with elsewhere, right?

class << self def chdir_semaphore @chdir_semaphore ||= Mutex.new end end

I think you're right, but are you sure that's threadsafe? If we have 2 threads that call chdir_semaphore before it is set, is it possible they both end up calling Mutex.new?

True. So maybe something like this then:

@chdir_semaphore = Mutex.new class << self attr_reader :chdir_semaphore end

(if I remember the context right - the `@chdir_semaphore call in normal class context will assign it to the class-level variable. Please try this out before trusting me blindly on this.)

@perlun I tested with your suggestion, it works 👍

perlun · 2018-07-17T19:04:15Z

lib/git/base.rb

+        end
+        Process.wait(pid)
+      else
+        # Windows and NetBSD 4 don't support fork()


Interesting anecdote, I wasn't aware of NetBSD 4 here. But OTOH, it's past EOL already. Maybe we can drop that reference and instead mention JRuby (which is actively used by many), since it also has problems with fork() on the JVM.

perlun · 2018-07-17T19:04:47Z

lib/git/base.rb

-      Dir.chdir(repo.path) do
-        return `du -s`.chomp.split.first.to_i
-      end
+      return `du -s #{repo.path}`.chomp.split.first.to_i


Very good fix, absolutely no reason to chdir in this case. 🤦‍♂️

perlun · 2018-07-17T19:05:35Z

lib/git/lib.rb

@@ -310,7 +311,7 @@ def branches_all
    def list_files(ref_dir)
      dir = File.join(@git_dir, 'refs', ref_dir)
      files = []
-      Dir.chdir(dir) { files = Dir.glob('**/*').select { |f| File.file?(f) } } rescue nil
+      files = Dir.glob(File.join(dir, '**/*')).select { |f| File.file?(f) }


I wonder if the rescue nil removal can be dangerous? Unfortunately not aware of its root cause of being there though.

Yeah, I was thinking about that too. I wasn't convinced it was needed, but it's probably safer since passing nil into File.file? or Dir.glob can raise an error

perlun · 2018-08-13T08:55:20Z

@taquitos Is this reviewable now? If so, please rebase and I'll give it a look ASAP.

Revert "355"

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. Fixes #355 Note: if you do async work in git.chdir all bets are off. - Reduce and isolate usage of Dir.chdir - fork() when using Dir.chdir, otherwise, use a mutex - wrap calls to in a semaphore - replace Dir.chdir with paths Signed-off-by: Joshua Liebowitz <taquitos@google.com>

taquitos · 2018-08-13T19:05:27Z

Ugh. Making new PR

[WIP]Start replacing Dir.chdir with paths

3d789f7

Instead of using chdir blocks, we should use absolute paths so that we can enable threaded usage of the library Fixes #355

taquitos commented Jun 25, 2018

View reviewed changes

Merge branch 'master' into 355

3ad8851

taquitos and others added 2 commits June 25, 2018 13:50

Merge branch 'master' into 355

ba1df97

fork() when using Dir.chdir, otherwise, use a mutex

f3f7518

Signed-off-by: Joshua Liebowitz <taquitos@google.com>

Merge pull request #1 from taquitos/355

f1cd6de

355

taquitos pushed a commit to fastlane/ci that referenced this pull request Jun 25, 2018

[WIP] GitRepos get individual work queues

c0637fa

Now that we’ve made progress on ruby-git/ruby-git#372 We can limit mutexes to be individual repo-scoped instead of 1 giant mutex for all.

taquitos mentioned this pull request Jun 25, 2018

[WIP] GitRepos get individual work queues fastlane/ci#1033

Closed

5 tasks

fork() when using Dir.chdir, otherwise, use a mutex

df31b57

Signed-off-by: Joshua Liebowitz <taquitos@google.com>

taquitos changed the title ~~[WIP]Start replacing Dir.chdir with paths~~ Isolate Dir.chdir to a new process, or mutex Jun 25, 2018

taquitos commented Jun 25, 2018

View reviewed changes

taquitos changed the title ~~Isolate Dir.chdir to a new process, or mutex~~ [WIP]Isolate Dir.chdir to a new process, or mutex Jun 25, 2018

wrap calls to in a semaphore

22ae609

Signed-off-by: Joshua Liebowitz <taquitos@google.com>

taquitos commented Jun 25, 2018

View reviewed changes

Merge pull request #2 from taquitos/355

5449c01

355

taquitos changed the title ~~[WIP]Isolate Dir.chdir to a new process, or mutex~~ Isolate Dir.chdir to a new process, or mutex Jun 27, 2018

Merge remote-tracking branch 'upstream/master'

ae1baa3

perlun requested changes Jul 17, 2018

View reviewed changes

stevenoneill mentioned this pull request Aug 2, 2018

bugfix to make dir.chdir threadsafe chef/mixlib-archive#29

Merged

3 tasks

Joshua Liebowitz and others added 4 commits August 13, 2018 09:44

Merge remote-tracking branch 'upstream/master'

e57f46f

Revert "355"

0d32576

Merge pull request #3 from taquitos/revert-2-355

a100a36

Revert "355"

Revert "355"

1cdc5fd

taquitos and others added 2 commits August 13, 2018 10:54

Merge pull request #4 from taquitos/revert-1-355

cdcc6a5

Revert "355"

taquitos closed this Aug 13, 2018

taquitos deleted the 355 branch August 13, 2018 19:05

xdmx mentioned this pull request Nov 20, 2022

Using one repo per thread results in errors #355

Closed

Isolate Dir.chdir to a new process, or mutex #372

Isolate Dir.chdir to a new process, or mutex #372

Uh oh!

Conversation

taquitos commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perlun commented Jun 25, 2018

Uh oh!

taquitos commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taquitos commented Jun 25, 2018

Uh oh!

perlun commented Jul 10, 2018

Uh oh!

taquitos commented Jul 17, 2018

Uh oh!

perlun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perlun commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taquitos commented Aug 13, 2018

Uh oh!

Uh oh!

taquitos commented Jun 25, 2018 •

edited

Loading

perlun commented Aug 13, 2018 •

edited

Loading