kartik-v / bootstrap-fileinput

An enhanced HTML 5 file input for Bootstrap 5.x/4.x./3.x with file preview, multiple selection, and more features.
http://plugins.krajee.com/file-input
Other
5.36k stars 2.4k forks source link

Ajax resumable upload endless loop, (and multple chunks file upload issue ?) #1557

Closed Kwaadpepper closed 4 years ago

Kwaadpepper commented 4 years ago

Prerequisites

Steps to reproduce the issue

~I have not found a sure way to reproduce this yet, but here is my config~ This can happen on demo page just upload a large file to force chunk upload, so larger than 2MB

$('#uploadInput').fileinput({
    language: uploadLocale,
    uploadAsync: true,
    enableResumableUpload: true,

    uploadUrl: actions.upload.url,
    uploadExtraData : {
        _token: 'tokenString',
        filePath: "${decodeURIComponent(getUrlLocationParameter('path'))}/"
    },

    showCaption: false, 
    showBrowse: false, 
    showPreview: true,
    showRemove: false, 
    showUpload: true, 
    showUploadStats: true,
    showCancel: null,
    showPause: null,
    showClose: false,
    showUploadedThumbs: true,
    showConsoleLogs: true,

    browseOnZoneClick: false,
    autoReplace: false,
    autoOrientImage: false, 
    autoOrientImageInitial: true,
    focusCaptionOnBrowse: true,
    focusCaptionOnClear: true,
    required: false,

    rtl: false,
    hideThumbnailContent: false,
    encodeUrl: true,
    generateFileId: null,
    previewClass: '',
    captionClass: '',
    frameClass: 'krajee-default',
    mainClass: 'file-caption-main',

    theme: "krajee-explorer",

    overwriteInitial: false,
    initialPreviewAsData: true,
    maxFileSize: 10000,
    removeFromPreviewOnError: true,
    previewFileType: 'any',

    fileSizeGetter: function(bytes) {
        i = Math.floor(Math.log(bytes) / Math.log(1024))
        sizes = [trans('B'), trans('KB'), trans('MB'), trans('GB'), trans('TB'), trans('PB'), trans('EB'), trans('ZB'), trans('YB')]
        return (bytes / Math.pow(1024, i)).toFixed(2) * 1 + ' ' + sizes[i]
    }
});

The server answer

Capture d’écran de 2020-05-12 13-29-40

  1. Setup a resumable upoad
  2. open the JS debugger
  3. set a breakpoint on the if statement of the processQueu method

    processQueue = function () {
        var config, xhr;
        if (self.ajaxCurrentThreads < self.maxAjaxThreads) {
            config = self.ajaxQueue.shift();
            if (typeof config !== 'undefined') {
                self.ajaxCurrentThreads++;
                xhr = $.ajax(config).done(function () {
                    clearInterval(self.ajaxQueueIntervalId);
                    self.ajaxCurrentThreads--;
                });
                self.ajaxRequests.push(xhr);
            }
        }
    };

    Expected behavior and actual behavior

This is the context, when i set the breakpoint: file has been uploaded, see the capture

Capture d’écran de 2020-05-12 13-38-15

As you can see, server answered "{\"chunkIndex\":\"4\",\"append\":true}"

I believe this is wanted as stated in the example https://plugins.krajee.com/file-input#ajax-resumable

I was expecting...

AllTimeouts to be ended.

Also if i anwer HTTP 500 and error message from the server see the following capture, it seems to be stuck. The loop still happens.

Capture d’écran de 2020-05-12 13-42-58

Environment

Browsers

Operating System

Libraries

Isolating the problem

Kwaadpepper commented 4 years ago

I have found this from server make the library to work on single chunk upload

Capture d’écran de 2020-05-12 14-35-13

whereas on multiple chunk upload

Capture d’écran de 2020-05-12 14-38-05

So i think there is a double issue here:

kartik-v commented 4 years ago

Thanks for the report... need to test and reproduce this -- a JS Fiddle or something to reproduce this will be ideal... You can also share a PR if you have a solution handy.

Kwaadpepper commented 4 years ago

Sure, Better than a fiddle, you can use your example https://plugins.krajee.com/file-resumable-uploads-demo#resumable-uploads-1, I can reproduce this there.

I'm also trying to sort this out. I'll make a PR if i can correct the bug here.

Kwaadpepper commented 4 years ago

Il have donne some tests. Ans this is caused by: 1- Ajax return fonction usage 2- intervals not bien correctly killed, because interval ids are lost firstly

I am still working on this

kartik-v commented 4 years ago

Thanks @Kwaadpepper ... do let know ... I did not got time to test this... will do that from my end as well a bit later. I have pushed in some other fixes to other issues and enhancements in the meanwhile - so you can use the latest code from the repo in your PR.

Kwaadpepper commented 4 years ago

This last push is working at least. This is hard to sort out. But as i see things, It should have a thread manager to synchronize all Intervals since no mechanism such as semaphores exists in JS.

I think this needs more work. I also noticed that general progressbar is only following the first file upload, then it set to 100%. while other files keeps uploading.

kartik-v commented 4 years ago

Thanks can you submit this over a PR -- I will try to look into this from my end as well.

kartik-v commented 4 years ago

I think this needs more work. I also noticed that general progressbar is only following the first file upload, then it set to 100%. while other files keeps uploading.

Yes need to look at the logic of updating the fileManager progress with your new changes.

kartik-v commented 4 years ago

Few suggestions I had in my mind... if we wanted a queueing library implementation for these ajax threads - we can also look at one of the following few inbuilt options in jquery instead of reinventing the wheel - (the thought I earlier add was basic where I had implemented earlier using setInterval as a quicker option):

... we can also probably implement a simple ajax queue manager object to manage the queuing of ajax threads and requests.

Kwaadpepper commented 4 years ago

You're right, I guess using jquery promises or differed would make it easier. I will try to workout something with that.

The goal is to make File Thread wait for chunksThread, himself to wait for Ajax Thread, in case of a resumable upload. Using a fifo Stack with jQuery.when would work I think. I need to get more familiar with these Jquery functions.

In any case, to follow your first approach, a file should have either: (A) a stack of chunk promises or deferred (chunk file upload) Or (B) a stack of ajax Promises (integral file upload).

A chunk promise would have a stack of ajax Promises.

Defered would be better since it has notification. I am referring to this article https://blog.engineering.publicissapient.fr/2012/11/28/les-objets-differes-et-les-promesses-en-jquery/

(sorry its in french)

Kwaadpepper commented 4 years ago

I spend the day making this small lib, think I will try to use that. https://codepen.io/Kwaadpepper/pen/zYvJroY?editors=0012

It has a dependency, https://www.npmjs.com/package/jquery-whenall, it's a small function https://gist.github.com/fearphage/4341799

It's basically a wrapper for Jquery promises. It allows stacking "Tasks" and then run them all at once of with a "MaxParallel" value;

If handles tasks pools (for uploaded files), each pool would contain "Tasks". It is also possible to run a single task.

All tasks can be run with a context;

kartik-v commented 4 years ago

This looks great. Think we could wrap both into this lib code as one optimized queue manager because they are small functions

Kwaadpepper commented 4 years ago

Sure, I'll try to finalize this if you're okay.

kartik-v commented 4 years ago

Sure... Will collab over this pull request.