From 05de1305a903ee4ce9c8c50fde53c552d5b90d51 Mon Sep 17 00:00:00 2001
From: Clay Gerrard <clay.gerrard@gmail.com>
Date: Thu, 27 Aug 2015 18:35:09 -0700
Subject: [PATCH] Make ssync_sender send valid chunked requests

The connect method of ssync_sender tells the remote connection that it's
going to send a valid HTTP chunked request, but if the remote end needs
to respond with an error of any kind sender throws HTTP right out the
window, picks up his ball, and closes the socket down hard - much to the
surprise of the eventlet.wsgi server who up to this point had been
playing along quite nicely with this 'SSYNC' nonsense assuming that
everyone here is consenting mature adults.

If you're going to make a "Transfer-Encoding: chunked" request have the
good decency to finish the job with a proper '0\r\n\r\n'. [1]

N.B. It might be possible to handle an error status during the
initialize_request phase with some sort of 100-continue support, but
honestly it's not entirely clear to me when the server isn't going to
close the connection if the client is still expected to send the body
[2] - further if the error comes later during missing_check or updates
we'll for sure want to send the chunk transfer termination line before
we close down the socket and this way we cover both.

1. Really, eventlet.wsgi shouldn't be so blasted brittle about this [3]
2. https://lists.w3.org/Archives/Public/ietf-http-wg/2005AprJun/0007.html
3. https://github.com/eventlet/eventlet/commit/c3ce3eef0b4d0dfdbfb1ec0186d4bb204fb8ecd5

Closes-Bug #1489587
Change-Id: Ic17c6c3075553f8cf6ef6213e62a00282f0d01cf
---
 swift/obj/ssync_sender.py            |  5 ++++-
 test/unit/obj/test_ssync_receiver.py | 31 +++++++++++++++++++++++++++-
 test/unit/obj/test_ssync_sender.py   |  3 +++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/swift/obj/ssync_sender.py b/swift/obj/ssync_sender.py
index c768bbfc82..cf6fcad6a4 100644
--- a/swift/obj/ssync_sender.py
+++ b/swift/obj/ssync_sender.py
@@ -82,7 +82,6 @@ class Sender(object):
                                       set(self.send_list))
                     can_delete_obj = dict((hash_, self.available_map[hash_])
                                           for hash_ in in_sync_hashes)
-                self.disconnect()
                 if not self.failures:
                     return True, can_delete_obj
                 else:
@@ -103,6 +102,8 @@ class Sender(object):
                     self.node.get('replication_ip'),
                     self.node.get('replication_port'),
                     self.node.get('device'), self.job.get('partition'))
+            finally:
+                self.disconnect()
         except Exception:
             # We don't want any exceptions to escape our code and possibly
             # mess up the original replicator code that called us since it
@@ -351,6 +352,8 @@ class Sender(object):
         Closes down the connection to the object server once done
         with the SSYNC request.
         """
+        if not self.connection:
+            return
         try:
             with exceptions.MessageTimeout(
                     self.daemon.node_timeout, 'disconnect'):
diff --git a/test/unit/obj/test_ssync_receiver.py b/test/unit/obj/test_ssync_receiver.py
index 9c757e0ae1..38654ffd61 100644
--- a/test/unit/obj/test_ssync_receiver.py
+++ b/test/unit/obj/test_ssync_receiver.py
@@ -31,7 +31,7 @@ from swift.common import utils
 from swift.common.swob import HTTPException
 from swift.obj import diskfile
 from swift.obj import server
-from swift.obj import ssync_receiver
+from swift.obj import ssync_receiver, ssync_sender
 from swift.obj.reconstructor import ObjectReconstructor
 
 from test import unit
@@ -1705,6 +1705,35 @@ class TestSsyncRxServer(unittest.TestCase):
     def tearDown(self):
         shutil.rmtree(self.tmpdir)
 
+    def test_SSYNC_disconnect(self):
+        node = {
+            'replication_ip': '127.0.0.1',
+            'replication_port': self.rx_port,
+            'device': 'sdb1',
+        }
+        job = {
+            'partition': 0,
+            'policy': POLICIES[0],
+            'device': 'sdb1',
+        }
+        sender = ssync_sender.Sender(self.daemon, node, job, ['abc'])
+
+        # kick off the sender and let the error trigger failure
+        with mock.patch('swift.obj.ssync_receiver.Receiver.initialize_request')\
+                as mock_initialize_request:
+            mock_initialize_request.side_effect = \
+                swob.HTTPInternalServerError()
+            success, _ = sender()
+        self.assertFalse(success)
+        stderr = six.StringIO()
+        with mock.patch('sys.stderr', stderr):
+            # let gc and eventlet spin a bit
+            del sender
+            for i in range(3):
+                eventlet.sleep(0)
+        self.assertNotIn('ValueError: invalid literal for int() with base 16',
+                         stderr.getvalue())
+
     def test_SSYNC_device_not_available(self):
         with mock.patch('swift.obj.ssync_receiver.Receiver.missing_check')\
                 as mock_missing_check:
diff --git a/test/unit/obj/test_ssync_sender.py b/test/unit/obj/test_ssync_sender.py
index 53f40c757a..211ab39c46 100644
--- a/test/unit/obj/test_ssync_sender.py
+++ b/test/unit/obj/test_ssync_sender.py
@@ -70,6 +70,9 @@ class NullBufferedHTTPConnection(object):
     def getresponse(*args, **kwargs):
         pass
 
+    def close(*args, **kwargs):
+        pass
+
 
 class FakeResponse(object):