[ruby-dev:39206] Re: [ruby-cvs:31917] Ruby:r24699 (trunk): * lib/tmpdir.rb (Dir.mktmpdir): removed thread race condition.

なかだです。

At Sat, 29 Aug 2009 13:10:30 +0900,
Yukihiro Matsumoto wrote in [ruby-dev:39198]:
> |ただ、Tempfile はもっと基本的なところで奇妙な感じがしますが。
> |
> |たとえば、テンポラリファイルを生成するときにロックディレクト
> |リを作るっていうのはあからさまに変だと思うんですが。どういう
> |意図なんでしょうね?
> 
> 歴史的な事情なんじゃないかと思います。当初はO_EXCL使ってませ
> んでしたから、確か。

昔はNFSだとあてにならないとかありませんでしたっけ。

> |実際のテンポラリファイルを作るときに O_EXCL を指定しているん
> |だからロックの必要はなくて、そこで成功するまで再挑戦するべき
> |だと思うんですが。
> |
> |そうしないと、そっちで衝突したらテンポラリファイルの生成自体
> |が失敗しちゃうし。
> 
> ですかねえ。パッチ作ってくださっても構いません。

そうなるとほとんどmktmpdirと同じになるような気がします。


Index: lib/tempfile.rb
===================================================================
--- lib/tempfile.rb	(revision 24713)
+++ lib/tempfile.rb	(working copy)
@@ -80,7 +80,5 @@ require 'thread'
 # mutex.
 class Tempfile < DelegateClass(File)
-  MAX_TRY = 10  # :nodoc:
-  @@cleanlist = []
-  @@lock = Mutex.new
+  include Dir::Tmpdir
 
   # call-seq:
@@ -132,64 +130,18 @@ class Tempfile < DelegateClass(File)
     if opts = Hash.try_convert(rest[-1])
       rest.pop
+      opts = [opts]
     end
-    tmpdir = rest[0] || Dir::tmpdir
-    if $SAFE > 0 and tmpdir.tainted?
-      tmpdir = '/tmp'
-    end
-
-    lock = tmpname = nil
-    n = failure = 0
-    @@lock.synchronize {
-      begin
-        begin
-          tmpname = File.join(tmpdir, make_tmpname(basename, n))
-          lock = tmpname + '.lock'
-          n += 1
-        end while @@cleanlist.include?(tmpname) or
-            File.exist?(lock) or File.exist?(tmpname)
-        Dir.mkdir(lock)
-      rescue
-        failure += 1
-        retry if failure < MAX_TRY
-        raise "cannot generate tempfile `#{tmpname}'"
-      end
-    }
 
-    @data = [tmpname]
-    @clean_proc = Tempfile.callback(@data)
+    @data = []
+    @clean_proc = self.class.callback(@data)
     ObjectSpace.define_finalizer(self, @clean_proc)
 
-    if opts.nil?
-      opts = []
-    else
-      opts = [opts]
+    create(basename, *rest) do |tmpname|
+      @data[0] = @tmpname = tmpname
+      @data[1] = @tmpfile = File.open(tmpname, File::RDWR|File::CREAT|File::EXCL, 0600, *opts)
     end
-    @tmpfile = File.open(tmpname, File::RDWR|File::CREAT|File::EXCL, 0600, *opts)
-    @tmpname = tmpname
-    @@cleanlist << @tmpname
-    @data[1] = @tmpfile
-    @data[2] = @@cleanlist
 
     super(@tmpfile)
-
-    # Now we have all the File/IO methods defined, you must not
-    # carelessly put bare puts(), etc. after this.
-
-    Dir.rmdir(lock)
-  end
-
-  def make_tmpname(basename, n)
-    case basename
-    when Array
-      prefix, suffix = *basename
-    else
-      prefix, suffix = basename, ''
-    end
-
-    t = Time.now.strftime("%Y%m%d")
-    th = Thread.current.object_id
-    path = "#{prefix}#{t}-#{$$}-#{th.to_s(36)}-#{rand(0x100000000).to_s(36)}-#{n}#{suffix}"
   end
-  private :make_tmpname
 
   # Opens or reopens the file with mode "r+".
@@ -270,6 +222,5 @@ class Tempfile < DelegateClass(File)
         File.unlink(@tmpname)
       end
-      @@cleanlist.delete(@tmpname)
-      # remove tmpname and cleanlist from callback
+      # remove tmpname from callback
       @data[0] = @data[2] = nil
       @data = @tmpname = nil
@@ -280,4 +231,10 @@ class Tempfile < DelegateClass(File)
   alias delete unlink
 
+  # Returns whether #unlink has been called on this Tempfile, and
+  # whether it succeeded.
+  def unlinked?
+    !@tmpname
+  end
+
   # Returns the full path name of the temporary file.
   # This will be nil if #unlink has been called.
@@ -302,6 +259,8 @@ class Tempfile < DelegateClass(File)
       pid = $$
       Proc.new {
+	# If we forked, then don't cleanup the temp files created by
+	# the parent process.
 	if pid == $$
-	  path, tmpfile, cleanlist = *data
+	  path, tmpfile = *data
 
 	  STDERR.print "removing ", path, "..." if $DEBUG
@@ -312,5 +271,4 @@ class Tempfile < DelegateClass(File)
 	  if path
 	    File.unlink(path) if File.exist?(path)
-	    cleanlist.delete(path) if cleanlist
 	  end
 
Index: lib/tmpdir.rb
===================================================================
--- lib/tmpdir.rb	(revision 24713)
+++ lib/tmpdir.rb	(working copy)
@@ -11,6 +11,12 @@ class Dir
   @@systmpdir = '/tmp'
 
-  begin
-    require 'Win32API'
+  if case RUBY_PLATFORM
+     when /mswin|mingw|cygwin/
+       begin
+         require 'Win32API'
+         true
+       rescue LoadError
+       end
+     end
     CSIDL_LOCAL_APPDATA = 0x001c
     max_pathlen = 260
@@ -31,5 +37,4 @@ class Dir
     temp = File.expand_path('temp', windir.untaint)
     @@systmpdir = temp if File.directory?(temp) and File.writable?(temp)
-  rescue LoadError
   end
 
@@ -97,38 +102,51 @@ class Dir
   #
   def Dir.mktmpdir(prefix_suffix=nil, tmpdir=nil)
-    case prefix_suffix
-    when nil
-      prefix = "d"
-      suffix = ""
-    when String
-      prefix = prefix_suffix
-      suffix = ""
-    when Array
-      prefix = prefix_suffix[0]
-      suffix = prefix_suffix[1]
+    path = Tmpdir.create(prefix_suffix || "d", tmpdir) {|n| Dir.mkdir(n, 0700)}
+    if block_given?
+      begin
+        yield path
+      ensure
+        FileUtils.remove_entry_secure path
+      end
     else
-      raise ArgumentError, "unexpected prefix_suffix: #{prefix_suffix.inspect}"
+      path
     end
-    tmpdir ||= Dir.tmpdir
-    t = Time.now.strftime("%Y%m%d")
-    n = nil
-    begin
-      path = "#{tmpdir}/#{prefix}#{t}-#{$$}-#{rand(0x100000000).to_s(36)}"
+  end
+
+  module Tmpdir
+    module_function
+
+    def make_tmpname(prefix_suffix, n)
+      case prefix_suffix
+      when String
+        prefix = prefix_suffix
+        suffix = ""
+      when Array
+        prefix = prefix_suffix[0]
+        suffix = prefix_suffix[1]
+      else
+        raise ArgumentError, "unexpected prefix_suffix: #{prefix_suffix.inspect}"
+      end
+      t = Time.now.strftime("%Y%m%d")
+      path = "#{prefix}#{t}-#{$$}-#{rand(0x100000000).to_s(36)}"
       path << "-#{n}" if n
       path << suffix
-      Dir.mkdir(path, 0700)
-    rescue Errno::EEXIST
-      n ||= 0
-      n += 1
-      retry
     end
 
-    if block_given?
+    def create(basename, tmpdir=nil)
+      if $SAFE > 0 and tmpdir.tainted?
+        tmpdir = '/tmp'
+      else
+        tmpdir ||= Dir.tmpdir
+      end
+      n = nil
       begin
-        yield path
-      ensure
-        FileUtils.remove_entry_secure path
+        path = File.join(tmpdir, make_tmpname(basename, n))
+        yield(path)
+      rescue Errno::EEXIST
+        n ||= 0
+        n += 1
+        retry
       end
-    else
       path
     end


-- 
--- 僕の前にBugはない。
--- 僕の後ろにBugはできる。
    中田 伸悦