Skip to content

Installer#build_extensions method is not thread safe #607

Closed
gnufied opened this Issue Jul 26, 2013 · 2 comments

2 participants

@gnufied
gnufied commented Jul 26, 2013

Bundler in certain cases calls this method concurrently and it does not appear to be thread safe. for now, we have worked around by monkey patching RG by using thread safe versions of Dir.chdir etc:

      require 'monitor'
      @chdir_monitor = Monitor.new
      def chdir(dir, &blk)
        @chdir_monitor.synchronize do
          Dir.chdir dir, &blk
        end
      end

      def pwd
        @chdir_monitor.synchronize do
          Dir.pwd
        end
      end

Think it would be a good idea to fix this in Rubygems itself.

@luislavena
RubyGems member

@gnufied hello, thank you for pointing this out.

I have a question, is the code you pointed out inside Bundler? or is your custom solution outside of it?

I believe no part of the extension compilation process was proofed to be threadsafe, so would like to look where inside Bundler this is being called concurrently (links to the code in GitHub will be great).

Thank you.

@gnufied
gnufied commented Jul 26, 2013

@luislavena https://github.com/bundler/bundler/pull/2567/files#L0R9 but my PR just refactors parallel worker pool code in bundler and makes it work on JRuby.

Original code is already checked into Bundler - https://github.com/bundler/bundler/blob/master/lib/bundler/gem_installer.rb#L9

On Bundler master it is currently only enabled on MRI (on *nix platform) and Windows.

@drbrain drbrain added a commit that referenced this issue Jul 30, 2013
@drbrain drbrain Move extension building to Gem::Ext::Builder
The rest of the extension building functionality is already located
under ext/ so it makes sense to move the driver to this existing empty
class.

This is groundwork for fixing #607 in a maintainable way.
931e4cf
@drbrain drbrain added a commit that closed this issue Jul 30, 2013
@drbrain drbrain Prevent extensions from building in parallel
Building an extension requires changing the path so commands executed
via a subprocess will work correctly.  In order to prevent parallel
extension builds from changing each other's directory a mutex is
required.

Fixes #607
ea1e46e
@drbrain drbrain closed this in ea1e46e Jul 30, 2013
@drbrain drbrain added a commit that referenced this issue Jul 30, 2013
@drbrain drbrain Reduce scope of CHDIR_MUTEX
As multiple extensions can't be built in parallel by bundler (the API of
Gem::Installer does not allow it) there is no need to include @ran_rake
in the mutex.

Updates #607 to match the 2.0 branch.
1dedd5e
@drbrain drbrain added a commit that referenced this issue Jul 30, 2013
@drbrain drbrain Prevent extensions from building in parallel
Building an extension requires changing the path so commands executed
via a subprocess will work correctly.  In order to prevent parallel
extension builds from changing each other's directory a mutex is
required.

Fixes #607
6cbf9b8
@drbrain drbrain added a commit that referenced this issue Sep 9, 2013
@drbrain drbrain Prevent extensions from building in parallel
Building an extension requires changing the path so commands executed
via a subprocess will work correctly.  In order to prevent parallel
extension builds from changing each other's directory a mutex is
required.

Fixes #607
05e9e55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.