Merge pull request #1370 from nextcloud/dav-fixassemblystreamperf

[uc] Improve chunk upload AssemblyStream performance

2 files changed, 94 insertions, 11 deletions

diff --git a/apps/dav/lib/Upload/AssemblyStream.php b/apps/dav/lib/Upload/AssemblyStream.php
index 5853adfa77b..3a18f91677d 100644
--- a/apps/dav/lib/Upload/AssemblyStream.php
+++ b/apps/dav/lib/Upload/AssemblyStream.php
@@ -50,6 +50,9 @@ class AssemblyStream implements \Icewind\Streams\File {
 	/** @var int */
 	private $size;
 
+	/** @var resource */
+	private $currentStream = null;
+
 	/**
 	 * @param string $path
 	 * @param string $mode
@@ -102,16 +105,36 @@ class AssemblyStream implements \Icewind\Streams\File {
 	 * @return string
 	 */
 	public function stream_read($count) {
+		do {
+			if ($this->currentStream === null) {
+				list($node, $posInNode) = $this->getNodeForPosition($this->pos);
+				if (is_null($node)) {
+					// reached last node, no more data
+					return '';
+				}
+				$this->currentStream = $this->getStream($node);
+				fseek($this->currentStream, $posInNode);
+			}
 
-		list($node, $posInNode) = $this->getNodeForPosition($this->pos);
-		if (is_null($node)) {
-			return null;
-		}
-		$stream = $this->getStream($node);
+			$data = fread($this->currentStream, $count);
+			// isset is faster than strlen
+			if (isset($data[$count - 1])) {
+				// we read the full count
+				$read = $count;
+			} else {
+				// reaching end of stream, which happens less often so strlen is ok
+				$read = strlen($data);
+			}
 
-		fseek($stream, $posInNode);
-		$data = fread($stream, $count);
-		$read = strlen($data);
+			if (feof($this->currentStream)) {
+				fclose($this->currentStream);
+				$this->currentNode = null;
+				$this->currentStream = null;
+			}
+			// if no data read, try again with the next node because
+			// returning empty data can make the caller think there is no more
+			// data left to read
+		} while ($read === 0);
 
 		// update position
 		$this->pos += $read;
diff --git a/apps/dav/tests/unit/Upload/AssemblyStreamTest.php b/apps/dav/tests/unit/Upload/AssemblyStreamTest.php
index ea875886af7..66aea40ef22 100644
--- a/apps/dav/tests/unit/Upload/AssemblyStreamTest.php
+++ b/apps/dav/tests/unit/Upload/AssemblyStreamTest.php
@@ -35,18 +35,78 @@ class AssemblyStreamTest extends \Test\TestCase {
 		$this->assertEquals($expected, $content);
 	}
 
+	/**
+	 * @dataProvider providesNodes()
+	 */
+	public function testGetContentsFread($expected, $nodes) {
+		$stream = \OCA\DAV\Upload\AssemblyStream::wrap($nodes);
+
+		$content = '';
+		while (!feof($stream)) {
+			$content .= fread($stream, 3);
+		}
+
+		$this->assertEquals($expected, $content);
+	}
+
 	function providesNodes() {
+		$data8k = $this->makeData(8192);
+		$dataLess8k = $this->makeData(8191);
 		return[
-			'one node only' => ['1234567890', [
+			'one node zero bytes' => [
+				'', [
+				$this->buildNode('0', '')
+			]],
+			'one node only' => [
+				'1234567890', [
 				$this->buildNode('0', '1234567890')
 			]],
-			'two nodes' => ['1234567890', [
+			'one node buffer boundary' => [
+				$data8k, [
+				$this->buildNode('0', $data8k)
+			]],
+			'two nodes' => [
+				'1234567890', [
 				$this->buildNode('1', '67890'),
 				$this->buildNode('0', '12345')
-			]]
+			]],
+			'two nodes end on buffer boundary' => [
+				$data8k . $data8k, [
+				$this->buildNode('1', $data8k),
+				$this->buildNode('0', $data8k)
+			]],
+			'two nodes with one on buffer boundary' => [
+				$data8k . $dataLess8k, [
+				$this->buildNode('1', $dataLess8k),
+				$this->buildNode('0', $data8k)
+			]],
+			'two nodes on buffer boundary plus one byte' => [
+				$data8k . 'X' . $data8k, [
+				$this->buildNode('1', $data8k),
+				$this->buildNode('0', $data8k . 'X')
+			]],
+			'two nodes on buffer boundary plus one byte at the end' => [
+				$data8k . $data8k . 'X', [
+				$this->buildNode('1', $data8k . 'X'),
+				$this->buildNode('0', $data8k)
+			]],
 		];
 	}
 
+	private function makeData($count) {
+		$data = '';
+		$base = '1234567890';
+		$j = 0;
+		for ($i = 0; $i < $count; $i++) {
+			$data .= $base[$j];
+			$j++;
+			if (!isset($base[$j])) {
+				$j = 0;
+			}
+		}
+		return $data;
+	}
+
 	private function buildNode($name, $data) {
 		$node = $this->getMockBuilder('\Sabre\DAV\File')
 			->setMethods(['getName', 'get', 'getSize'])