refactor httpfile and remove layering violation

Splitting calls to backend.create_open + create_close between
files is also extremely confusing and error-prone.

Hopefully nobody actually depends on some attributes
we've removed.

3 files changed, 37 insertions, 76 deletions

diff --git a/lib/mogilefs/httpfile.rb b/lib/mogilefs/httpfile.rb
index f415bba..22692e7 100644
--- a/lib/mogilefs/httpfile.rb
+++ b/lib/mogilefs/httpfile.rb
@@ -27,16 +27,7 @@ class MogileFS::HTTPFile < StringIO
 
   attr_reader :uri
 
-  ##
-  # The key for this file.  This key won't represent a real file until you've
-  # called #close.
-
-  attr_reader :key
-
-  ##
-  # The class of this file.
-
-  attr_reader :class
+  attr_reader :devid
 
   ##
   # The big_io name in case we have file > 256M
@@ -46,46 +37,20 @@ class MogileFS::HTTPFile < StringIO
   attr_accessor :streaming_io
 
   ##
-  # Works like File.open.  Use MogileFS::MogileFS#new_file instead of this
-  # method.
-
-  def self.open(*args)
-    fp = new(*args)
-    fp.set_encoding(Encoding::BINARY) if fp.respond_to?(:set_encoding)
-
-    return fp unless block_given?
-
-    begin
-      yield fp
-    ensure
-      fp.close
-    end
-  end
-
-  ##
   # Creates a new HTTPFile with MogileFS-specific data.  Use
   # MogileFS::MogileFS#new_file instead of this method.
 
-  def initialize(mg, fid, klass, key, dests, content_length)
-    super ''
-    @mg = mg
-    @fid = fid
-    @uri = @devid = nil
-    @klass = klass
-    @key = key
-    @big_io = nil
-    @streaming_io = nil
-
+  def initialize(dests, content_length)
+    super ""
+    @streaming_io = @big_io = @uri = @devid = nil
     @dests = dests
     @tried = {}
-
-    @socket = nil
   end
 
   ##
   # Writes an HTTP PUT request to +sock+ to upload the file and
   # returns file size if the socket finished writing
-  def upload(devid, uri)
+  def upload(devid, uri) # :nodoc:
     file_size = length
     sock = MogileFS::Socket.tcp(uri.host, uri.port)
 
@@ -109,47 +74,35 @@ class MogileFS::HTTPFile < StringIO
                  "Content-Length: #{length}\r\n\r\n#{string}")
     end
 
-    line = sock.gets or
+    case line = sock.timed_read(23, "")
+    when %r{^HTTP/\d\.\d\s+(2\d\d)\s} # success!
+      file_size
+    when nil
       raise EmptyResponseError, 'Unable to read response line from server'
-
-    if line =~ %r%^HTTP/\d+\.\d+\s+(\d+)% then
-      case $1.to_i
-      when 200..299 then # success!
-      else
-        raise BadResponseError, "HTTP response status from upload: #{$1}"
-      end
+    when %r{^HTTP/\d\.\d\s+(\d+)}
+      raise BadResponseError, "HTTP response status from upload: #$1"
     else
       raise UnparseableResponseError, "Response line not understood: #{line}"
     end
+    ensure
+      sock.close if sock && ! sock.closed?
+  end
 
-    @mg.backend.create_close(:fid => @fid, :devid => devid,
-                             :domain => @mg.domain, :key => @key,
-                             :path => uri.to_s, :size => file_size)
-    file_size
-  end # def upload
-
-  def close
-    try_dests = @dests.dup
-    last_err = nil
-
-    loop do
-      devid, url = try_dests.shift
-      devid && url or break
-
-      uri = URI.parse(url)
+  def commit
+    errors = nil
+    @dests.each do |devid, path|
       begin
-        bytes = upload(devid, uri)
+        uri = URI.parse(path)
+        bytes_uploaded = upload(devid, uri)
         @devid, @uri = devid, uri
-        return bytes
-      rescue SystemCallError, Errno::ECONNREFUSED, MogileFS::Timeout,
-             EmptyResponseError, BadResponseError,
-             UnparseableResponseError => err
-        last_err = @tried[url] = err
+        return bytes_uploaded
+      rescue => e
+        errors ||= []
+        errors << "#{path} failed with #{e.message} (#{e.class})"
       end
     end
 
-    raise last_err ? last_err : NoStorageNodesError
+    raise NoStorageNodesError,
+          "all paths failed with PUT: #{errors.join(', ')}", []
   end
-
 end
-
diff --git a/lib/mogilefs/mogilefs.rb b/lib/mogilefs/mogilefs.rb
index e0df888..267c896 100644
--- a/lib/mogilefs/mogilefs.rb
+++ b/lib/mogilefs/mogilefs.rb
@@ -87,7 +87,7 @@ class MogileFS::MogileFS < MogileFS::Client
   #
   # The +block+ operates like File.open.
 
-  def new_file(key, klass = nil, bytes = 0, &block) # :yields: file
+  def new_file(key, klass = nil, bytes = 0) # :yields: file
     raise MogileFS::ReadOnlyError if readonly?
     opts = { :domain => @domain, :key => key, :multi_dest => 1 }
     opts[:class] = klass if klass && klass != "default"
@@ -111,8 +111,16 @@ class MogileFS::MogileFS < MogileFS::Client
     when nil, '' then
       raise MogileFS::EmptyPathError
     when /^http:\/\// then
-      MogileFS::HTTPFile.open(self, res['fid'], klass, key,
-                              dests, bytes, &block)
+      httpfile = MogileFS::HTTPFile.new(dests, bytes)
+      yield httpfile
+      rv = httpfile.commit
+      @backend.create_close(:fid => res['fid'],
+                            :devid => httpfile.devid,
+                            :domain => @domain,
+                            :key => key,
+                            :path => httpfile.uri.to_s,
+                            :size => rv)
+      rv
     else
       raise MogileFS::UnsupportedPathError,
             "paths '#{dests.inspect}' returned by backend is not supported"
diff --git a/test/test_mogilefs.rb b/test/test_mogilefs.rb
index 34cde58..f85548f 100644
--- a/test/test_mogilefs.rb
+++ b/test/test_mogilefs.rb
@@ -444,7 +444,7 @@ class TestMogileFS__MogileFS < TestMogileFS
       'path' => "http://127.0.0.1:#{t.port}/path",
     }
 
-    assert_raises MogileFS::HTTPFile::BadResponseError do
+    assert_raises MogileFS::HTTPFile::NoStorageNodesError do
       @client.store_content 'new_key', 'test', 'data'
     end
   end