Merge pull request #14251 from tomasz-grobelny/upload_reliability_improvements

Limit number of simultaneous MKCOL requests to server to increase upload reliability

3 files changed, 120 insertions, 68 deletions

diff --git a/apps/files/js/file-upload.js b/apps/files/js/file-upload.js
index e0b274cdc76..a56668c493f 100644
--- a/apps/files/js/file-upload.js
+++ b/apps/files/js/file-upload.js
@@ -194,6 +194,9 @@ OC.FileUpload.prototype = {
 		var data = this.data;
 		var file = this.getFile();
 
+		if (self.aborted === true) {
+			return $.Deferred().resolve().promise();
+		}
 		// it was a folder upload, so make sure the parent directory exists already
 		var folderPromise;
 		if (file.relativePath) {
@@ -243,8 +246,10 @@ OC.FileUpload.prototype = {
 		}
 
 		// wait for creation of the required directory before uploading
-		$.when(folderPromise, chunkFolderPromise).then(function() {
-			data.submit();
+		return Promise.all([folderPromise, chunkFolderPromise]).then(function() {
+			if (self.aborted !== true) {
+				data.submit();
+			}
 		}, function() {
 			self.abort();
 		});
@@ -295,6 +300,7 @@ OC.FileUpload.prototype = {
 		}
 		this.data.abort();
 		this.deleteUpload();
+		this.aborted = true;
 	},
 
 	/**
@@ -317,7 +323,7 @@ OC.FileUpload.prototype = {
 		if (response.errorThrown) {
 			// attempt parsing Sabre exception is available
 			var xml = response.jqXHR.responseXML;
-			if (xml.documentElement.localName === 'error' && xml.documentElement.namespaceURI === 'DAV:') {
+			if (xml && xml.documentElement.localName === 'error' && xml.documentElement.namespaceURI === 'DAV:') {
 				var messages = xml.getElementsByTagNameNS('http://sabredav.org/ns', 'message');
 				var exceptions = xml.getElementsByTagNameNS('http://sabredav.org/ns', 'exception');
 				if (messages.length) {
@@ -554,7 +560,15 @@ OC.Uploader.prototype = _.extend({
 		var self = this;
 		_.each(uploads, function(upload) {
 			self._uploads[upload.data.uploadId] = upload;
-			upload.submit();
+		});
+		self.totalToUpload = _.reduce(uploads, function(memo, upload) { return memo+upload.getFile().size; }, 0);
+		var semaphore = new OCA.Files.Semaphore(5);
+		var promises = _.map(uploads, function(upload) {
+			return semaphore.acquire().then(function(){
+				return upload.submit().then(function(){
+					semaphore.release();
+				});
+			});
 		});
 	},
 
@@ -861,7 +875,6 @@ OC.Uploader.prototype = _.extend({
 				autoUpload: false,
 				sequentialUploads: false,
 				limitConcurrentUploads: 10,
-				//singleFileUploads is on by default, so the data.files array will always have length 1
 				/**
 				 * on first add of every selection
 				 * - check all files of originalFiles array with files in dir
@@ -886,15 +899,6 @@ OC.Uploader.prototype = _.extend({
 					// can't link directly due to jQuery not liking cyclic deps on its ajax object
 					data.uploadId = upload.getId();
 
-					// we need to collect all data upload objects before
-					// starting the upload so we can check their existence
-					// and set individual conflict actions. Unfortunately,
-					// there is only one variable that we can use to identify
-					// the selection a data upload is part of, so we have to
-					// collect them in data.originalFiles turning
-					// singleFileUploads off is not an option because we want
-					// to gracefully handle server errors like 'already exists'
-
 					// create a container where we can store the data objects
 					if ( ! data.originalFiles.selection ) {
 						// initialize selection and remember number of files to upload
@@ -1030,7 +1034,7 @@ OC.Uploader.prototype = _.extend({
 
 					self.removeUpload(upload);
 
-					if (data.textStatus === 'abort') {
+					if (data.textStatus === 'abort' || data.errorThrown === 'abort') {
 						self.showUploadCancelMessage();
 					} else if (status === 412) {
 						// file already exists
@@ -1134,14 +1138,15 @@ OC.Uploader.prototype = _.extend({
 				});
 				fileupload.on('fileuploadprogressall', function(e, data) {
 					self.log('progress handle fileuploadprogressall', e, data);
-					var progress = (data.loaded / data.total) * 100;
+					var total = self.totalToUpload;
+					var progress = (data.loaded / total) * 100;
 					var thisUpdate = new Date().getTime();
 					var diffUpdate = (thisUpdate - lastUpdate)/1000; // eg. 2s
 					lastUpdate = thisUpdate;
 					var diffSize = data.loaded - lastSize;
 					lastSize = data.loaded;
 					diffSize = diffSize / diffUpdate; // apply timing factor, eg. 1MiB/2s = 0.5MiB/s, unit is byte per second
-					var remainingSeconds = ((data.total - data.loaded) / diffSize);
+					var remainingSeconds = ((total - data.loaded) / diffSize);
 					if(remainingSeconds >= 0) {
 						bufferTotal = bufferTotal - (buffer[bufferIndex]) + remainingSeconds;
 						buffer[bufferIndex] = remainingSeconds; //buffer to make it smoother
@@ -1164,7 +1169,7 @@ OC.Uploader.prototype = _.extend({
 					}
 					self._setProgressBarText(h, h, t('files', '{loadedSize} of {totalSize} ({bitrate})' , {
 							loadedSize: humanFileSize(data.loaded),
-							totalSize: humanFileSize(data.total),
+							totalSize: humanFileSize(total),
 							bitrate: humanFileSize(data.bitrate / 8) + '/s'
 						}));
 					self._setProgressBarValue(progress);
diff --git a/apps/files/js/operationprogressbar.js b/apps/files/js/operationprogressbar.js
index efeea4ad78f..d07cea91235 100644
--- a/apps/files/js/operationprogressbar.js
+++ b/apps/files/js/operationprogressbar.js
@@ -32,8 +32,9 @@
 		},
 
 		hideCancelButton: function() {
+			var self = this;
 			$('#uploadprogresswrapper .stop').fadeOut(function() {
-				this.$el.trigger(new $.Event('resized'));
+				self.$el.trigger(new $.Event('resized'));
 			});
 		},
 
diff --git a/apps/files/tests/js/fileUploadSpec.js b/apps/files/tests/js/fileUploadSpec.js
index 0d2c9eb0697..b7955f3b837 100644
--- a/apps/files/tests/js/fileUploadSpec.js
+++ b/apps/files/tests/js/fileUploadSpec.js
@@ -84,33 +84,41 @@ describe('OC.Upload tests', function() {
 	}
 
 	describe('Adding files for upload', function() {
-		it('adds file when size is below limits', function() {
+		it('adds file when size is below limits', function(done) {
 			var result = addFiles(uploader, [testFile]);
 			expect(result[0]).not.toEqual(null);
-			expect(result[0].submit.calledOnce).toEqual(true);
+			result[0].submit.callsFake(function(){
+				expect(result[0].submit.calledOnce).toEqual(true);
+				done();
+			});
 		});
-		it('adds file when free space is unknown', function() {
+		it('adds file when free space is unknown', function(done) {
 			var result;
 			$('#free_space').val(-2);
 
 			result = addFiles(uploader, [testFile]);
-
 			expect(result[0]).not.toEqual(null);
-			expect(result[0].submit.calledOnce).toEqual(true);
-			expect(failStub.notCalled).toEqual(true);
+			result[0].submit.callsFake(function(){
+				expect(result[0].submit.calledOnce).toEqual(true);
+				expect(failStub.notCalled).toEqual(true);
+				done();
+			});
 		});
-		it('does not add file if it exceeds free space', function() {
+		it('does not add file if it exceeds free space', function(done) {
 			var result;
 			$('#free_space').val(1000);
 
+			failStub.callsFake(function(){
+				expect(failStub.calledOnce).toEqual(true);
+				expect(failStub.getCall(0).args[1].textStatus).toEqual('notenoughspace');
+				expect(failStub.getCall(0).args[1].errorThrown).toEqual(
+					'Not enough free space, you are uploading 5 KB but only 1000 B is left'
+				);
+				setTimeout(done, 0);
+			});
 			result = addFiles(uploader, [testFile]);
 
 			expect(result[0]).toEqual(null);
-			expect(failStub.calledOnce).toEqual(true);
-			expect(failStub.getCall(0).args[1].textStatus).toEqual('notenoughspace');
-			expect(failStub.getCall(0).args[1].errorThrown).toEqual(
-				'Not enough free space, you are uploading 5 KB but only 1000 B is left'
-			);
 		});
 	});
 	describe('Upload conflicts', function() {
@@ -158,38 +166,60 @@ describe('OC.Upload tests', function() {
 
 			fileList.destroy();
 		});
-		it('does not show conflict dialog when no client side conflict', function() {
+		it('does not show conflict dialog when no client side conflict', function(done) {
+			$('#free_space').val(200000);
+			var counter = 0;
+			var fun = function() {
+				counter++;
+				if(counter != 2) {
+					return;
+				}
+				expect(result[0].submit.calledOnce).toEqual(true);
+				expect(result[1].submit.calledOnce).toEqual(true);
+				setTimeout(done, 0);
+			};
 			var result = addFiles(uploader, [{name: 'noconflict.txt'}, {name: 'noconflict2.txt'}]);
+			result[0].submit.callsFake(fun);
+			result[1].submit.callsFake(fun);
 
 			expect(conflictDialogStub.notCalled).toEqual(true);
-			expect(result[0].submit.calledOnce).toEqual(true);
-			expect(result[1].submit.calledOnce).toEqual(true);
+
 		});
-		it('shows conflict dialog when no client side conflict', function() {
+		it('shows conflict dialog when no client side conflict', function(done) {
+			var counter = 0;
+			conflictDialogStub.callsFake(function(){
+				counter++;
+				if(counter != 3) {
+					return $.Deferred().resolve().promise();
+				}
+				setTimeout(function() {
+					expect(conflictDialogStub.callCount).toEqual(3);
+					expect(conflictDialogStub.getCall(1).args[0].getFileName())
+						.toEqual('conflict.txt');
+					expect(conflictDialogStub.getCall(1).args[1])
+						.toEqual({ name: 'conflict.txt', mimetype: 'text/plain', directory: '/' });
+					expect(conflictDialogStub.getCall(1).args[2]).toEqual({ name: 'conflict.txt' });
+
+					// yes, the dialog must be called several times...
+					expect(conflictDialogStub.getCall(2).args[0].getFileName()).toEqual('conflict2.txt');
+					expect(conflictDialogStub.getCall(2).args[1])
+						.toEqual({ name: 'conflict2.txt', mimetype: 'text/plain', directory: '/' });
+					expect(conflictDialogStub.getCall(2).args[2]).toEqual({ name: 'conflict2.txt' });
+
+					expect(result[0].submit.calledOnce).toEqual(false);
+					expect(result[1].submit.calledOnce).toEqual(false);
+					expect(result[2].submit.calledOnce).toEqual(true);
+					done();
+				}, 0);
+			});
 			var result = addFiles(uploader, [
 				{name: 'conflict.txt'},
 				{name: 'conflict2.txt'},
 				{name: 'noconflict.txt'}
 			]);
 
-			expect(conflictDialogStub.callCount).toEqual(3);
-			expect(conflictDialogStub.getCall(1).args[0].getFileName())
-				.toEqual('conflict.txt');
-			expect(conflictDialogStub.getCall(1).args[1])
-				.toEqual({ name: 'conflict.txt', mimetype: 'text/plain', directory: '/' });
-			expect(conflictDialogStub.getCall(1).args[2]).toEqual({ name: 'conflict.txt' });
-
-			// yes, the dialog must be called several times...
-			expect(conflictDialogStub.getCall(2).args[0].getFileName()).toEqual('conflict2.txt');
-			expect(conflictDialogStub.getCall(2).args[1])
-				.toEqual({ name: 'conflict2.txt', mimetype: 'text/plain', directory: '/' });
-			expect(conflictDialogStub.getCall(2).args[2]).toEqual({ name: 'conflict2.txt' });
-
-			expect(result[0].submit.calledOnce).toEqual(false);
-			expect(result[1].submit.calledOnce).toEqual(false);
-			expect(result[2].submit.calledOnce).toEqual(true);
 		});
-		it('cancels upload when skipping file in conflict mode', function() {
+		it('cancels upload when skipping file in conflict mode', function(done) {
 			var fileData = {name: 'conflict.txt'};
 			var uploadData = addFiles(uploader, [
 				fileData
@@ -197,11 +227,14 @@ describe('OC.Upload tests', function() {
 
 			var upload = new OC.FileUpload(uploader, uploadData[0]);
 			var deleteStub = sinon.stub(upload, 'deleteUpload');
+			deleteStub.callsFake(function(){
+				expect(deleteStub.calledOnce).toEqual(true);
+				done();
+			});
 
 			uploader.onSkip(upload);
-			expect(deleteStub.calledOnce).toEqual(true);
 		});
-		it('overwrites file when choosing replace in conflict mode', function() {
+		it('overwrites file when choosing replace in conflict mode', function(done) {
 			var fileData = {name: 'conflict.txt'};
 			var uploadData = addFiles(uploader, [
 				fileData
@@ -210,12 +243,14 @@ describe('OC.Upload tests', function() {
 			expect(uploadData[0].submit.notCalled).toEqual(true);
 
 			var upload = new OC.FileUpload(uploader, uploadData[0]);
-
+			uploadData[0].submit.callsFake(function(){
+				expect(upload.getConflictMode()).toEqual(OC.FileUpload.CONFLICT_MODE_OVERWRITE);
+				expect(uploadData[0].submit.callCount).toEqual(1);
+				done();
+			});
 			uploader.onReplace(upload);
-			expect(upload.getConflictMode()).toEqual(OC.FileUpload.CONFLICT_MODE_OVERWRITE);
-			expect(uploadData[0].submit.calledOnce).toEqual(true);
 		});
-		it('autorenames file when choosing replace in conflict mode', function() {
+		it('autorenames file when choosing replace in conflict mode', function(done) {
 			// needed for _.defer call
 			var clock = sinon.useFakeTimers();
 			var fileData = {name: 'conflict.txt'};
@@ -227,20 +262,31 @@ describe('OC.Upload tests', function() {
 
 			var upload = new OC.FileUpload(uploader, uploadData[0]);
 			var getResponseStatusStub = sinon.stub(upload, 'getResponseStatus');
+			var counter = 0;
+			uploadData[0].submit.callsFake(function(){
+				counter++;
+				if(counter===1)
+				{
+					expect(upload.getConflictMode()).toEqual(OC.FileUpload.CONFLICT_MODE_AUTORENAME);
+					expect(upload.getFileName()).toEqual('conflict (2).txt');
+					expect(uploadData[0].submit.calledOnce).toEqual(true);
+					getResponseStatusStub.returns(412);
+					uploader.fileUploadParam.fail.call($dummyUploader[0], {}, uploadData[0]);
+					clock.tick(500);
+				}
+				if(counter===2)
+				{
+					expect(upload.getFileName()).toEqual('conflict (3).txt');
+					expect(uploadData[0].submit.calledTwice).toEqual(true);
+
+					clock.restore();
+					done();
+				}
+			});
 
 			uploader.onAutorename(upload);
-			expect(upload.getConflictMode()).toEqual(OC.FileUpload.CONFLICT_MODE_AUTORENAME);
-			expect(upload.getFileName()).toEqual('conflict (2).txt');
-			expect(uploadData[0].submit.calledOnce).toEqual(true);
 
 			// in case of server-side conflict, tries to rename again
-			getResponseStatusStub.returns(412);
-			uploader.fileUploadParam.fail.call($dummyUploader[0], {}, uploadData[0]);
-			clock.tick(500);
-			expect(upload.getFileName()).toEqual('conflict (3).txt');
-			expect(uploadData[0].submit.calledTwice).toEqual(true);
-
-			clock.restore();
 		});
 	});
 });