Downloading a lot of big files in parallel leads to stack overflows (Range Error)

[ssledorze]

Bug Report

Problem

Downloading a lot (10) big files in parallel leads to stack overflows (Range Error)

What is expected to happen?

It should download without creating stack overflows

What does actually happen?

This is the interesting part:
When the plugin does some transfer, it generate a lot of progress messages between the plugin native part and its js part.
Those events generates nested calls for cordova callbacks for the Js part to interpret the native events.
Those nested calls become so long that they just stack overflow..

Information

I've tested it on Android (chased for days)

Command or Code

Just download 10 big files in parallel on a not so fast phone (tested on an emulator but hinted by production crashs)

Environment, Platform, Device

Nexus 7 API 29 emulator

Version information

Latest version of all.

Possible Solution

Add an additional parameter to hint not to send back progress event.
Propagate this param to the Android / iOS parts and do not construct/send progress events
It would have the side effect of preventing the onprogress to work.

Checklist

• I searched for existing GitHub issues
• I updated all Cordova tooling to most recent version
• I included all the necessary information above

[abelc-opteameo]

I concur, this was maddening to debug!, just getting a plain opaque call stack overflow with no traceability...

Something about the way those VM files are generated during parallel downloads just doesn't look right, we've seen them taking 2000+ lines in callbacks.

Besides making the onprogress callback optional, shouldn't we also have some form of control over the MAX_BUFFER_SIZE at https://github.com/apache/cordova-plugin-file-transfer/blob/master/src/android/FileTransfer.java#L72 ? It seems to also influence the number of default progress callbacks.

[ssledorze]

@breautek thanks for the unmatched lib, I'm wondering if what I propose would be acceptable for you.
If not, do you find the issue relevant? In that case how might we fix it?

[breautek]

@breautek thanks for the unmatched lib, I'm wondering if what I propose would be acceptable for you.
If not, do you find the issue relevant? In that case how might we fix it?

I had a glance at it, but waited on my comment since it was a draft. I think the PR is could be a workaround but I would be hesitant to add the feature in which we would have to support long term.

I think the issue is relevant. The way the callback code is generated is definitely a problem. I see no reason why these callbacks would be nested under any circumstance. It would be interesting to know if this can be reproduced on iOS.

[ssledorze]

@breautek I agree about the nesting calls.
But I think changing the communication scheme between the Plugins and Cordova, while the best move, is a very wide change to Cordova and I think won't happen over night (I may be wrong)..

The way the PR is done is not that intrusive but I agree it's an additional feature to maintain.
Let me know your thinking; I need the fix to hit prod ASAP (bloodbath there..)

I think I can live with a fork to maintain but ideally we would highlight the issue to other devs and offer a path to a non crashing solution.
Actually without a solution in that Plugin they would simply leave it for another one or fork as there's no other workaround ATM (even if the wrong design is on cordova's side).

[breautek]

But I think changing the communication scheme between the Plugins and Cordova, while the best move, is a very wide change to Cordova

Maybe, I'm not too familiar with this part of the codebase myself, but I question why we don't see this behaviour (or at least run into stack overflows) with other keep alive callbacks. For example, one of my apps uses device motion that does a callback approximately at 32Hz frequency, and the user's logging session can go on for hours.

If keep alive callbacks trigger a nested callback look like we see here, I would have expected to see the same issue, but I haven't. This leads me to believe that something about the file transfer plugin is doing something weird.

[ssledorze]

@breautek the impact on the call stack depends on the number of nesting which depends the speed at which the nesting are issued and executed (in user land) and the stack consumption of each (user land) callback.

Having a high frequency of adds up but is not the only variable at play here..

Note in my case; no (End) user land code was touched, only the code in the Js part of the FileTransfer plugin (which does almost nothing as noProgress callback was registered).
So it looks like in the end, everything is in between cordova and FileTransfer..

I'll deploy from the fork.. at least for short term..

[KleggerKoder]

I solved this by throttling the progress events being sent rather then another solution that disabled them altogether.
#292

[ssledorze]

Happy to collect more proof of the issue..