Skip to content

Fix race-condition during vips load #114

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 service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 17, 2017

Conversation

felixbuenemann
Copy link
Contributor

@felixbuenemann felixbuenemann commented Jul 15, 2017

This avoids the problematic lazy-loading of the GI module which goes
horribly wrong if it is attempted by two threads at once.

If one really wants to delay laoding of the gi module, it is possible to
set $vips_skip_autoload = true before requiring vips or simply don't
require vips until it is needed.

The race condition can be reproduced with the following code:

require "vips"
10.times do
  fork do
    q=Queue.new
    th=[]
    th << Thread.new { q << (Vips::Access::SEQUENTIAL rescue $!) }
    th << Thread.new { q << (Vips::Image.new_from_array([0]) rescue $!) }
    th.map(&:join)
    puts q.pop.inspect
    puts q.pop.inspect
  end
end
Process.waitall

Without this fix, the code will print random exceptions like:

#<ArgumentError: Vips.init: wrong number of arguments (0 for 1)>
#<NoMethodError: undefined method `new_from_array' for Vips::Image:Class
#<NameError: uninitialized constant Vips::Image

This fixes #107.

This avoids the problematic lazy-loading of the GI module which goes
horribly wrong if it is attempted by two threads at once.

If one really wants to delay laoding of the gi module, it is possible to
set $vips_skip_autoload = true before requiring vips or simply don't
require vips until it is needed.
@jcupitt
Copy link
Member

jcupitt commented Jul 15, 2017

That looks much better. Could it need a Thread.exclusive as well?

@felixbuenemann
Copy link
Contributor Author

Sorry too many bottles of wine, could you elaborate?

I tried using a mutex, but it didn't yreally help, because we basically would have to avoid entering const/method missing as soon as another init is running or we're screwed.

@felixbuenemann
Copy link
Contributor Author

So to be more specific: I tried putting a mutex aroud init with a boolean to abort early so it only ran once, but that still didn’t fix the issue so I chose to get rid of the lazy loading which has the benefit of being cow friendly because we allocate memory before the fork.

I also changed the method name because it was clashing with a method defined by GI and I found it very confusing to have the same method with different signatures and behavior depending on the object lifecycle. (That’s why you see the wrong number of arguments exception above.)

@jcupitt jcupitt merged commit 2669676 into libvips:master Jul 17, 2017
@jcupitt
Copy link
Member

jcupitt commented Jul 17, 2017

Hi again, you're right, I'd had too many bottles of wine too. This is great, let's merge!

I'll update the gem later today.

@jcupitt
Copy link
Member

jcupitt commented Jul 17, 2017

OK, 1.0.6 is up and has this fix, plus a much faster #to_a. Thanks again!

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.

undefined method 'new_from_file' for Vips::Image:Class
2 participants