From f37832aed753df3b629eb7d3ee343287c76625d8 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 18 Dec 2013 17:48:29 -0800
Subject: [PATCH] Fix loop in svnserve workflow for large binaries

Summary: If you push a large binary and the data crosses multiple data frames, we can end up in a loop in the parser.

Test Plan:
After this change, I was able to push a 95MB binary in 7s, which seems reasonable:

  >>> orbital ~/repos/INIS $ svn st
  A       large2.bin
  >>> orbital ~/repos/INIS $ ls -alh
  total 390648
  drwxr-xr-x   6 epriestley  admin   204B Dec 18 17:14 .
  drwxr-xr-x  98 epriestley  admin   3.3K Dec 16 11:19 ..
  drwxr-xr-x   7 epriestley  admin   238B Dec 18 17:14 .svn
  -rw-r--r--   1 epriestley  admin    80B Dec 18 15:07 README
  -rw-r--r--   1 epriestley  admin    95M Dec 18 16:53 large.bin
  -rw-r--r--   1 epriestley  admin    95M Dec 18 17:14 large2.bin
  >>> orbital ~/repos/INIS $ time svn commit -m 'another large binary'
  Adding  (bin)  large2.bin
  Transmitting file data .
  Committed revision 25.

  real	0m7.215s
  user	0m5.327s
  sys	0m0.407s
  >>> orbital ~/repos/INIS $

There may be room to improve this by using `PhutilRope`.

Reviewers: wrotte, btrahan, wotte

Reviewed By: wotte

CC: aran

Differential Revision: https://secure.phabricator.com/D7798
---
 .../DiffusionSubversionWireProtocol.php       |  4 +++
 ...iffusionSubversionWireProtocolTestCase.php | 25 +++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php b/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php
index 934969f792..e5645859a4 100644
--- a/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php
+++ b/src/applications/diffusion/protocol/DiffusionSubversionWireProtocol.php
@@ -75,6 +75,10 @@ final class DiffusionSubversionWireProtocol extends Phobject {
         }
       } else if ($this->state == 'bytes') {
         $new_data = substr($this->buffer, 0, $this->expectBytes);
+        if (!strlen($new_data)) {
+          // No more bytes available yet, wait for more data.
+          break;
+        }
         $this->buffer = substr($this->buffer, strlen($new_data));
 
         $this->expectBytes -= strlen($new_data);
diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php b/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php
index 28b6473eb8..088f7222ed 100644
--- a/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php
+++ b/src/applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php
@@ -61,6 +61,31 @@ final class DiffusionSubversionWireProtocolTestCase
       ));
   }
 
+  public function testSubversionWireProtocolPartialFrame() {
+    $proto = new DiffusionSubversionWireProtocol();
+
+    // This is primarily a test that we don't hang when we write() a frame
+    // which straddles a string boundary.
+    $msg1 = $proto->writeData('( duck 5:qu');
+    $msg2 = $proto->writeData('ack ) ');
+
+    $this->assertEqual(array(), ipull($msg1, 'structure'));
+    $this->assertEqual(
+      array(
+        array(
+          array(
+            'type' => 'word',
+            'value' => 'duck',
+          ),
+          array(
+            'type'=> 'string',
+            'value' => 'quack',
+          ),
+        ),
+      ),
+      ipull($msg2, 'structure'));
+  }
+
   private function assertSameSubversionMessages($string, array $structs) {
     $proto = new DiffusionSubversionWireProtocol();