Enhance and fix resumable and ajax queues

[Kwaadpepper]

Scope

This pull request includes a

• Bug fix
• New feature
• Translation

Changes

The following changes were made

• changed fileManager.processed to fileManager.filesProcessed
  this makes more sense, for code readability
  this is an optional change

• changed Object.keys to $h.getObjectKeys method
  this is an optional change
  https://stackoverflow.com/questions/5223/length-of-a-javascript-object

• added getObjectKeys method in $h

• added getObjectSize method in $h

• added taskManager in $h

• added $.whenAll jquery extension to handle running all chunks a once if
  resumableUploadOptions.maxThreads is equal to 0
  I think this could be removed, since we could want
  chunks to always be uploaded "maxThreads" at a time.
  If so some code in taskManager.TasksPool.runAllTasksParallels
  could be remove the "// if run all at once" part with the jquery
  extension

• ajax requests are stand alone tasks (promises)

• resumable uploads are a pool of tasks foreach chunks
  they are runned in parrallel

I wont remove the last setInterval for upload: function
this is too much work, and code readability is poor in my opinion.
It would benefit to be reactored using an object approach
instead of a functionnal programming.

There should also be a unique upload method: resumable.
It would be adapted for each options

Related Issues

#1557

[kartik-v]

Thanks I will try to optimize this a bit.

[Kwaadpepper]

I did not remove all interval use.
This should be the purpose if this PR.

[kartik-v]

Yes I agree - are you working on that part? Let know... I thought I could optimize the Task manager code a bit. But maybe will wait till you confirm posting your updates.

[Kwaadpepper]

Yes, my bad. I will let tout know when its ready.

[Kwaadpepper]

Think this is ready for you to check and test.
I think there would be some improvement to be done with the progress bar.
I left some comments.

[kartik-v]

Thanks. I am checking this and have tried to optimize the TaskManager lib. The initial issue I am seeing is that pause and resume functionality seems broken somewhere as well as updating of the main progress bar. Will need to check and test in detail if anything else.

[kartik-v]

There needs to be a pause and resume feature - need to check your task pool implementation - currently it has only cancel and success.

[Kwaadpepper]

You're right looks like there is an issue with resuming. It restarts uploading all chunks. As for the main progress bar i'm not sure what is causing this issue i tried to keep original code as much as possible. I need to check where it is updated.

Checking this.

[Kwaadpepper]

Ok i fixed things for resume uploads. I tested resume. It seems ok to me.

I noticed a bug though. When an upload starts for multiple files. I you pause, then browse and add a file. It's size is not added to fm.totalSize, So the general progress bar percent calculation will be at 100% when this last added file will be starting to be uploaded. This is a new bug unrelated to this PR. I checked i can reproduce this on your example https://plugins.krajee.com/file-resumable-uploads-demo#resumable-uploads-1.

[Kwaadpepper]

As for tasks. It can't be paused of course as well as Ajax queries since they are promises. But undone chunks (tasks) can be re-added to the queue. This is the actual behavior. So there is no pause related code in task manager.

I you want so I can squash the PR.

[kartik-v]

I you want so I can squash the PR.

Thanks - you can.

[Kwaadpepper]

done.

[kartik-v]

As for the main progress bar i'm not sure what is causing this issue i tried to keep original code as much as possible. I need to check where it is updated.

Thanks. I am not finding issues with the main progress bar updating in the new code ... can you let know - what is the issue you are facing with the main progress bar update?

Will check the other parts in the meanwhile - adding a new file to a paused resumable.

[kartik-v]

I think the task manager code in itself can be optimized to a more compressed one than it exists now. I will do it myself at the end once we get the others tested.

[Kwaadpepper]

Thanks. I am not finding issues with the main progress bar updating in the new code ... can you let know - what is the issue you are facing with the main progress bar update?

My last commit fixed the main progress bar issue i had.

I think the task manager code in itself can be optimized to a more compressed one than it exists now. I will do it myself at the end once we get the others tested.

Sure, there is code we can pull out. For example. the jquey plugin introducing $.whenall is not necessary if you don't allow max thread to be 0 ( upload all at once). I wrote code to handle that. But if you consider there is always a max number and a min number for ajax queries to run in parallel than you can remove $.whenAll plugin and if statement on line 660: runAllTasksParallels method.

[Kwaadpepper]

This could be removed eventually if you won't allow maxParallelsTaskNumber to be 0

[Kwaadpepper]

This could be removed eventually if you don't allow maxParallelsTaskNumber to be 0

[kartik-v]

There is one issue I am seeing on the pause and resume. When you pause it once and then resume it works fine. But when you pause and resume more than once for the same file - the code runs into an issue and enters this console.log... need to check.

[Kwaadpepper]

OKay i'll look at that.

[Kwaadpepper]

I am seeing some issues on my project that needs to be inverstigated:
Events are triggered when cancelling and pausing file upload (resumable) such as

bootstrap-fileinput: "uploadInput": 0: abort. Error Details: . fileinput.js:2153
filechunkajaxerror
filechunkcomplete

This is because hxr promise is rejected ( calling abort method ), It is not handled properly.

[kartik-v]

Yes abort of xhr is expected in case of cancel ... to terminate the ajax requests in process..., but for pause and resume I feel the resumableManager init must not happen unlike how it happens during cancel or a fresh upload.

So interestingly --- I took up your PR modification - you did on intervals ... the last updated one before you introduced the queue... I modified that code a bit for pause and resume...seems to work well for most part... only need to check the new file added during pause state...

[Kwaadpepper]

Yes i could make it work using interval, but it seems to me it wasn't a good solution. I feel like using promises is a more neat solution. It is up to you after all to decide what you want to use.

Yes abort of xhr is expected in case of cancel ... to terminate the ajax requests in process...,

This should not raise filechunkajaxerror or filechunkcomplete event in the process that was my point

[Kwaadpepper]

There is one issue I am seeing on the pause and resume. When you pause it once and then resume it works fine. But when

rm.stack is growing more and more when pause and resume few times this is the cause.
I will fix this if you want to use the task manager solution. I you prefer to use intervals as my previous PR i will leave it as it is and let you finish this work. As you wish. Fixed

[Kwaadpepper]

but for pause and resume I feel the resumableManager init must not happen unlike how it happens during cancel or a fresh upload.

I had this taught also. It happens that you are calling reset only when a fresh upload would occur. this is not really intuitive. Imao, code could be restructured in a more object oriented style. It would be easier to maintain. Maybe using coffeescript ? But this is a lot of work.

[kartik-v]

Great. Will have a look at this.

[kartik-v]

I had this taught also. It happens that you are calling reset only when a fresh upload would occur. this is not really intuitive. Imao, code could be restructured in a more object oriented style. It would be easier to maintain. Maybe using coffeescript ? But this is a lot of work.

I have been thinking of major revamp to the library as well . Initially this library was created as a bootstrap extension for file input and upload and features kept getting added with resumable uploads coming up recently. Meanwhile, we had bootstrap versions going from 2.x, then 3.x and now bootstrap 4.x and plugin changed to cater to these releases and throughout has been also dependent on the jQuery library.

If you are interested we can collaborate on a new branch or even a new repo to create a rehash of this library with latest javascript or coffeescript so that it works across all other frameworks.

[Kwaadpepper]

Sure I would like to collab on my free time. I am also working on my own side project ( a laravel file manager ), so working on my free time.

a rehash of this library with latest javascript or coffeescript so that it works across all other frameworks.

Which specific framework are you referring to ?

I am a Php full stack dev, and I have some experience with C# and other compiled code, but I am not a professional JS dev. I would have some suggestions though. But please give us some guidelines first.

In the meantime would you like to finish this PR first ?

[kartik-v]

Which specific framework are you referring to ?

I meant, make it extensible to be used in other javascript frameworks beyond jQuery - e.g. react, angular etc.. (or make jQuery optional) if needed. Which means work with pure javascript - but it needs a bit of rethink. Anyway this is a separate project to take up and can be done in free time.

In the meantime would you like to finish this PR first ?

Yes I have taken your code and done some updates and final changes - and now it seems to work fine - but I am doing some testing on this at my end and will close the PR if all goes fine.

[Kwaadpepper]

Nice.

I meant, make it extensible to be used in other javascript frameworks beyond jQuery - e.g. react, angular etc.. (or make jQuery optional) if needed. Which means work with pure javascript - but it needs a bit of rethink.

I see, removing jQuery could be done in vanilla JS of course. But as you said before if we don't want to reinvent the wheel, the question would be which browser you want to support. I guess not under ie9 or may be ie10 ?

[kartik-v]

Enhancement done and pushed. I have taken and refactored your code and simplified / optimized the queue functionality. Have a relook from your end on your testing and let know as well.

[Kwaadpepper]

I just pulled last master branch. I ran into this:

After an upload failed, the main progress bar was not updated. Server answered 500 for a chunk upload. I am looking at this. Nothing is running in this capture, it is stuck like that.

To reproduce that: upload a large file, pause the upload, then make server answer 500 and resume upload. After all failed. The main progress bar will be set to cancelled. But when the other chunks will be called and answer given the progress bar will be updated.

This case is: if the upload is cancelled (main progress bar set to cancel), then ajax answers must be ignored, or it will update the UI like upload is continuing.
When the first ajax retry failed 3 times. Upload is aborted. But other ajax answers are pending. They need to be ignored if upload is cancelled.

[Kwaadpepper]

With this capture, we see the upload ended, but the UI is not updated correctly. So there is an issue with handling errors.

[Kwaadpepper]

sendAjax.{fnSuccess,fnError} methods should be aware that setProcessed was previously called, ot the other way around, setProcessed should not have been called till all chunks returned.

I think tp.run, callback method is run before sendAjax.fnError in JS queue. So the Task manager will think all task are done, but another task will be inserted just after that.

It looks to be some kind of race condition. To be sure to reproduce this testing on local host, you should reduce bandwidth using firefox debugger.

[kartik-v]

Will check in sometime on this. please share your findings in the meanwhile.

[Kwaadpepper]

Solved the isse with this patch, tasksCount was not updated dynamically when ajax quieries were failing and adding more tasks.

diff --git a/js/fileinput.js b/js/fileinput.js
index f185262..0976606 100755
--- a/js/fileinput.js
+++ b/js/fileinput.js
@@ -696,7 +696,7 @@
                     tp.run = function (maxThreads) {
                         var i = 0, failed = false, task, tasksList = $h.getObjectKeys(tp.tasks).map(function (key) {
                             return tp.tasks[key];
-                        }), tasksCount = tasksList.length, tasksDone = [], deferred = $.Deferred(), enqueue, callback;
+                        }), tasksDone = [], deferred = $.Deferred(), enqueue, callback;
 
                         if (tp.cancelled) {
                             tp.cancelledDeferrer.resolve();
@@ -751,7 +751,7 @@
                                 tp.cancelledDeferrer.resolve();
                                 return;
                             }
-                            if (tasksDone.length === tasksCount) {
+                            if (tasksDone.length === tp.size()) {
                                 if (failed) {
                                     deferred.reject.apply(null, tasksDone);
                                 } else {

[Kwaadpepper]

I just ran through this also

When uploading, just cancel at any time to reproduce this.
it concerns this line pool.addTask(pool.size() + 1, function (deferrer) {

This is because the pool is removed when abort.

[Kwaadpepper]

diff --git a/js/fileinput.js b/js/fileinput.js
index f185262..f020296 100755
--- a/js/fileinput.js
+++ b/js/fileinput.js
@@ -696,7 +696,7 @@
                     tp.run = function (maxThreads) {
                         var i = 0, failed = false, task, tasksList = $h.getObjectKeys(tp.tasks).map(function (key) {
                             return tp.tasks[key];
-                        }), tasksCount = tasksList.length, tasksDone = [], deferred = $.Deferred(), enqueue, callback;
+                        }), tasksDone = [], deferred = $.Deferred(), enqueue, callback;
 
                         if (tp.cancelled) {
                             tp.cancelledDeferrer.resolve();
@@ -751,7 +751,7 @@
                                 tp.cancelledDeferrer.resolve();
                                 return;
                             }
-                            if (tasksDone.length === tasksCount) {
+                            if (tasksDone.length === tp.size()) {
                                 if (failed) {
                                     deferred.reject.apply(null, tasksDone);
                                 } else {
@@ -1504,6 +1504,10 @@
                         self._raise('filechunkbeforesend', [id, index, retry, fm, rm, outData]);
                     };
                     fnSuccess = function (data, textStatus, jqXHR) {
+                        if (self.cancelling) {
+                            deferrer.reject('cancelling');
+                            return;
+                        }
                         outData = self._getOutData(fd, jqXHR, data);
                         var paramNames = self.uploadParamNames, chunkIndex = paramNames.chunkIndex || 'chunkIndex',
                             opts = self.resumableUploadOptions, params = [id, index, retry, fm, rm, outData];
@@ -1531,6 +1535,10 @@
                         }
                     };
                     fnError = function (jqXHR, textStatus, errorThrown) {
+                        if (self.cancelling) {
+                            deferrer.reject('cancelling');
+                            return;
+                        }
                         outData = self._getOutData(fd, jqXHR);
                         rm.error = errorThrown;
                         rm.logAjaxError(jqXHR, textStatus, errorThrown);
@@ -1541,7 +1549,9 @@
                         deferrer.reject('try failed');
                     };
                     fnComplete = function () {
-                        self._raise('filechunkcomplete', [id, index, retry, fm, rm, self._getOutData(fd)]);
+                        if (!self.cancelling) {
+                            self._raise('filechunkcomplete', [id, index, retry, fm, rm, self._getOutData(fd)]);
+                        }
                     };
                     self._ajaxSubmit(fnBefore, fnSuccess, fnComplete, fnError, fd, id, rm.fileIndex);
                 }

[Kwaadpepper]

This patch solves both to last issues,
But the UI is not fully updated :

This issue was causing the symptom I described you earlier : filechunkcomplete and other ajax events should not be raised if upload is cancelled.
This issue remains for pausing a resumable file upload,
filechunkajaxerror
filechunkcomplete
are raised when pausing the upload.

This is not preventing the plugin to work though.

[kartik-v]

Thanks.. I will fix all of these and the filechunk events for aborted scenarios via a separate issue tracking that.

[kartik-v]

All combined together and fixed via #1561. Please check and report anything over that issue so we can track that. Thanks.