File::EXCL is not thread safe #827

Closed
minad opened this Issue Jun 25, 2013 · 13 comments

Projects

None yet

2 participants

@minad
minad commented Jun 25, 2013

You can also use this snippet:

require 'fileutils'

def create_file(name, value)
  ::File.open(name, ::File::WRONLY | ::File::CREAT | ::File::EXCL) do |f|
    f.write(value)
  end
  true
rescue Errno::EEXIST
  false
end

def create_thread(name)
  Thread.new do
    2000.times do |i|
      if create_file("data/#{i}", name)
        raise 'Concurrent create' if File.read("data/#{i}") != name
      end
      Thread.pass if rand(100) >= 99
    end
  end
end

FileUtils.mkdir('data')

a = create_thread('a')
b = create_thread('b')
c = create_thread('c')
a.join
b.join
c.join

FileUtils.rm_rf('data')
@headius
Member
headius commented Jun 25, 2013

Superseded by #828.

@headius headius closed this Jun 25, 2013
@headius
Member
headius commented Jun 25, 2013

Oops...I thought this was related to locking. Reopening.

@headius headius reopened this Jun 25, 2013
@headius
Member
headius commented Jun 25, 2013

The spec is flawed. The files are opened for exclusive access, written to and closed. At that point, there's no further exclusive access. The subsequent read can (and obviously does) occur after another thread has done the same exclusive open + write + close.

If you want to test this properly, the subsequent read of the file contents must also occur inside the File.open block, because otherwise there's nothing preventing another thread from coming in and overwriting the file before the read happens.

Filing as Invalid. Reopen if you are able to show an actual JRuby issue.

@headius headius closed this Jun 25, 2013
@minad
minad commented Jun 25, 2013

@headius I think you misunderstood. The argument O_CREAT | O_EXCL ensures that a file is created exclusively. The open by the next thread should fail. The snippet tests atomic file creation. Please read the UNIX man page man 2 open.

So this is still an issue. It works correctly under MRI.

@headius
Member
headius commented Jun 25, 2013

Yes, but the file is opened and then closed before the subsequent read that fails. Once a file is closed, there's no exclusive access anymore.

@minad
minad commented Jun 25, 2013

The issue is that the file is overwritten and this should not be allowed. O_EXCL | O_CREAT means exclusive or atomic create.

@minad
minad commented Jun 25, 2013

The issue is not about exclusive access, but about atomic create.

@headius
Member
headius commented Jun 25, 2013

Oh, right right. I'm getting my signals crossed with the locking issue still. Reopening to examine again.

@headius headius reopened this Jun 25, 2013
@minad
minad commented Jun 25, 2013

Therefore I opened two issues :)

@headius
Member
headius commented Jun 25, 2013

Ok, I think I see the problem in JRuby. We are creating the file using the appropriate exclusive-create APIs, but we do not raise an error if an exclusive create was not possible. Should be an easy fix, and this is a good find.

@minad
minad commented Jun 25, 2013

Cool, thank you!

@headius headius added a commit that closed this issue Jun 25, 2013
@headius headius Properly raise when a file cannot be created exclusively.
Our logic did not atomically ensure a file would be created
exclusively. It did a pre-create check to see if the file existed
and would raise an error if exclusive creation was requested. If
the file did not exist, it proceeded to do the proper atomic
create call but ignore the return value. Another thread (or
process) could jump in between these two operations and create the
file. Our logic would then proceed to open a file it intended to
have created exclusively but which was actually created by another
thread or process.

The fix is simple: raise if an exclusive create fails and
exclusive creation was requested.

Fixes #827, and a rubyspec is in rubyspec/rubyspec#231.
fb1bd76
@headius headius closed this in fb1bd76 Jun 25, 2013
@minad
minad commented Jun 25, 2013

that was fast! great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment