Feature: Throttle listener notifications

[phiamo]

Hi, Thanks for the amazing plugin! However i noticed while downloading larger files (~50 to 100mb) and tring to use the progress listener, that so many capacitor notifyListeners calls are created, that this slowed down the whole app as well as the download itself. when throttling this to 1 per second everything wokrs very well. maybe it would be a good idea to have the throttle interval configurable and disableable? Cheers Phil

[thomasvidas]

This PR was deleted due to changing the default branch name from master to main. Not because it wouldn't be a good feature 😄. I would not be opposed to adding this feature behind a request flag, but it would need to have parity on iOS as well and not just Android

[phiamo]

Arg.. 😉 didnt notice the name change. If necessary we can prepare that. But it didt face the same prob on ios ... yet

Thomas Vidas @.***> schrieb am Do., 18. Nov. 2021, 05:04:

This PR was deleted due to changing the default branch name from master to main. Not because it wouldn't be a good feature 😄. I would not be opposed to adding this feature behind a request flag, but it would need to have parity on iOS as well and not just Android

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/capacitor-community/http/pull/195#issuecomment-972508206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABSTO2NTPWAYUNVCN6CM23UMR3MLANCNFSM5HQ3ENYA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[jkbz64]

@thomasvidas

I would not be opposed to adding this feature behind a request flag, but it would need to have parity on iOS as well and not just Android

Also probably parity with web since its unthrottled too if we go for the perfect score

@phiamo

I have nothing against throttling by using intervals (its simple and works) but as an alternative to adding new feature flag I would propose running progress emit asynchronically and chaining promises/async tasks while cancelling all the tasks after the current running one (to avoid backlog) / calling emit only when emit task ended calling so that way we ensure only the latest one is being emitted and shouldn't block as much as synchronic notifyListeners. I haven't really tested that since it worked okay for me (I did the initial progress implementation) without throttling but I would try to go this way so we can avoid additional flag if it's possible (assuming it's more efficient/slightly less than interval but still much better than unthrottled).

[phiamo]

@jkbz64 sounds like a fantastic solution, i am not a java programmer so this is out of my scope :/ but i'll be happy to help wher i can

[jkbz64]

@phiamo I made simple android async throttle implementation, could you check if this commit fixes the issue for you? If it fixes the issue, the web implmentation should be trivial, for ios i'm not really sure.... https://github.com/jkbz64/http/commit/bfeb1e073a59647d921c29c9dbee7d08e6f18fcc (try out the alternative with Thread.sleep if Thread.yield doesn't make a difference)

[phiamo]

@jkbz64 thanks a lot for this beatyful implementation! the basic problem of too many messages emitted, though, is not solved:

My feeling is its emiting soo many messages and somehow android webview and capacitor is not abled to handle that:

2021-11-24 10:44:38.507 7938-7938/org.dwbn.recordings E/chromium: [ERROR:aw_browser_terminator.cc(149)] Renderer process (7980) crash detected (code 5).
2021-11-24 10:44:38.520 7938-7938/org.dwbn.recordings A/chromium: [FATAL:crashpad_client_linux.cc(667)] Render process (7980)'s crash wasn't handled by all associated  webviews, triggering application crash.
2021-11-24 10:44:38.521 7938-7938/org.dwbn.recordings A/libc: Fatal signal 5 (SIGTRAP), code -6 (SI_TKILL) in tid 7938 (dwbn.recordings), pid 7938 (dwbn.recordings)
2021-11-24 10:44:38.535 1228-1228/? I/AppBase: AppBase.onTrimMemory():615 onTrimMemory(): 10
2021-11-24 10:44:38.536 1228-1228/? I/GoogleInputMethodService: GoogleInputMethodService.onTrimMemory():4225 onTrimMemory(): 10
2021-11-24 10:44:38.540 1746-1746/? I/A: Trimming objects from memory, since app is in the background.
2021-11-24 10:44:38.545 4239-4239/? I/Finsky: [2] lpb.onTrimMemory(1): Memory trim requested to level 80
2021-11-24 10:44:38.549 4239-4239/? I/Finsky: [2] lpa.accept(3): Flushing in-memory image cache
2021-11-24 10:44:38.562 7288-7317/? W/Bugle: TextClassifierLibManagerImpl: Reclaiming memory at level: 40
2021-11-24 10:44:38.574 8163-8163/? I/crash_dump32: obtaining output fd from tombstoned, type: kDebuggerdTombstone
2021-11-24 10:44:38.576 281-281/? I/tombstoned: received crash request for pid 7938
2021-11-24 10:44:38.577 8163-8163/? I/crash_dump32: performing dump of process 7938 (target tid = 7938)
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: Build fingerprint: 'google/sdk_gphone_x86/generic_x86_arm:11/RSR1.201013.001/6903271:user/release-keys'
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: Revision: '0'
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: ABI: 'x86'
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: Timestamp: 2021-11-24 10:44:38+0100
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: pid: 7938, tid: 7938, name: dwbn.recordings  >>> org.dwbn.recordings <<<
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: uid: 10155
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: signal 5 (SIGTRAP), code -6 (SI_TKILL), fault addr --------
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG: Abort message: '[FATAL:crashpad_client_linux.cc(667)] Render process (7980)'s crash wasn't handled by all associated  webviews, triggering application crash.
    '
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG:     eax 00000000  ebx c58a48d8  ecx c590b874  edx 00000001
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG:     edi ff8d1888  esi ff8d1418
2021-11-24 10:44:38.580 8163-8163/? A/DEBUG:     ebp ff8d1868  esp ff8d1410  eip c0b5587a
2021-11-24 10:44:38.699 8163-8163/? A/DEBUG: backtrace:

i also think this might be connected with debug logging of capacitor, since i can reproduce this dump even with all of my code commented out, just by connecting the chrome inspect js debugger to the webview.

and per emit in android one addListener debug statement is printed to the console until ... it crashs

if i again add a delay like in the following snipplet, everything works like a charm: :angel:

        long lastEmit = new Date().getTime();

        while ((len = connectionInputStream.read(buffer)) > 0) {
            fileOutputStream.write(buffer, 0, len);

            bytes += len;

            if (bytes == maxBytes) continue;
            if (progressFuture.isDone() && lastEmit + 300 < new Date().getTime()) {
                progressTask.update(bytes);
                progressFuture = ProgressTask.executor.submit(progressTask);
                lastEmit = new Date().getTime();
            }
        }

i am not sure where you see the responsibility for the underlying problem, imho it would be nice if the http plugin would throttle this so that the developer experience is not tainted, but maybe documenting this maybe even edge case would suffice ?

for sure i found out, if i use a ngzone.run inside that listener without throttling the webview crashes even faster. so i use a dedicated subject and throttle this in the js code now. works well, until i enable debug and connect chrome ;)

[jkbz64]

@phiamo Thanks for looking into this

Just want to point out, my idea is not set in stone and I'm not in the power to decide, just wanted to see if this method would fix your performance issues and we could iterate upon that together.

i am not sure where you see the responsibility for the underlying problem

I haven't really done much research about this, just knew from the start the frequency of the requests might be too much for some cases. I assumed the problem was just that notifyListeners is blocking the download loop for too long. In my implementation I was trying only to address slowdown of the download, not to solve the "problem" of too many emits.

the basic problem of too many messages emitted, though, is not solved

In some cases "too many emits" might be helpful in other not so much. For a file that downloads in less than 300ms by using your method we will get emit only once so because of that (at least I think) @thomasvidas doesn't want to make it non-configurable (and kinda hassle, for every file smaller you might want to configure delay separately). If we want to use the emit delay we are back to having to introduce new options parameter which I wanted to avoid entirely, and if that doesn't work out we might as well just drop the async idea and do it your way

if i again add a delay like in the following snipplet, everything works like a charm

If we really want to add some kind of "delay" to the async way I would just plug in some really small non-configurable value to the Thread.sleep (like 30/50) in ProgressTask instead of Thread.yield, that reduced my emits from 250calls to 15-20calls and shouldn't affect most downloads but that is still opinionated (the 30/50 delay) which I don't like.

Also I just noticed, your initial (the one from PR) implementation would not [always] emit the completion progress event, which might be helpful for someone (e.g 250kb/250kb downloaded).