Transfer is seriously strangled while browser tab is in background

[jimmywarting]

What operating system and Node.js version?
Dosen't affect Node

What version of WebTorrent?
0.103.x

What browser and version? (if using WebTorrent in the browser)
Chrome v72

What did you expect to happen?
Expected process.nextTick to not use setTimeout

What actually happened?
The problem is with process browserified version of process.nextTick it uses a setTimeout(fn, 0) as a fallback. The setTimeout is delayed well enough in foreground to strangle the speed from 6-7000 kb/s down to just 400 kb/s when one tab is hidden "to save resources". if you instead use two windows the speed will be fast (normal) again.

setTimeout(fn, 0) can potentially be used, however as it is clamped to 4ms for timers nested more than 5 deep per the HTML spec, it does not make for a suitable polyfill for the natural immediacy of setImmediate. - MDN

Downloading from hybrid torrents works fine cuz they uses node nextTick version.
So to test this you basically need to upload some large files to instant.io and open up the sharable instant.io link in a new tab or new window

This affect users who want to download something and watch cat videos while its uploading/downloading

[RangerMauve]

What would be a better alternative to process.nextTick? It might be possible to detect the browser environment and reassign it to something better or do a PR against browserify.

[jimmywarting]

What would be a better alternative to process.nextTick?

If possible don't use nextTick at all in both memory-chunk-store and immediate-chunk-store and possible other places where it's not needed? not sure if streams uses nextTick also.

To me it looks like:

• PeerA request a piece from PeerB
• PeerB calls store.get()
• store calls memory store.get()
• memory store.get waits 4ms and then return it to immediate store
• immediate waits 4ms and then gives that to bittorent-protocol

now you waisted 8ms or more. Both storage layer don't use the same eventLoop when nextTicket is called in both of them it's more like a chain reaction then a parallel execution

It seems to me that this is also almost all pointless b/c of process.nextTick() turns into setTimeout(fn, 0 || 4)

webtorrent/lib/torrent.js

Lines 46 to 47 in 5b7c5ee

	// IndexedDB chunk stores used in the browser benefit from maximum concurrency
	const FILESYSTEM_CONCURRENCY = process.browser ? Infinity : 2

IndexedDB chunk stores will be wrapped in immediate-chunk-store so when you create a Buffer or Blob it will exit the current eventLoop and exit the current transaction?

---

I don't get what the reason is for using nextTick at all in the stores. Can someone explain what that would be good for?

[RangerMauve]

As I understand the use of nextTick is important in order to make sure all callbacks that are supposed to be invoked asynchronously, are guaranteed to be invoked asynchronously. This helps make async code more consistent and helps avoid race conditions in async code.

Maybe browserify should use this setImmediate polyfill instead of nextTick?

[RangerMauve]

The code would need to be changed in the module that Browserify uses for process.

I think that Promise.resolve().then(cb).catch((e) => setTimeout(()=> throw e,0)) might be closer to what nextTick is supposed to do than setImmediate or setTimeout

[jimmywarting]

As I understand the use of nextTick is important in order to make sure all callbacks that are supposed to be invoked asynchronously, are guaranteed to be invoked asynchronously. This helps make async code more consistent and helps avoid race conditions in async code.

I kinda know that already and i know what process.nextTick is suppose to solve, but i wonder is it really needed in the stores?

I saw that Browserified version earlier - should have linked to it earlier

found some solutions here also: https://codepen.io/rafaelcastrocouto/pen/gDFxt
found it at https://stackoverflow.com/questions/19906947/is-there-anything-faster-than-settimeout-and-requestanimationframe

seems like mutation observer is the fastest
From previous experience I have had - Promise are pretty slow performance wise, read something before that promise makes 3 jumps back and forth between the compiler and javascript - that is why promise can be slow

[jimmywarting]

actually setTimeout(fn, 0) is clamped to 4ms but delayed even further when tab is hidden

[RangerMauve]

I think this is more of an issue for Browserify to update their nextTick implementation, and I'm sure they'd welcome a PR or might have some issues already discussing this.

[calvinmetcalf]

so the default process.nextTick does indeed use setTimeout, but only in situation of process.nextTick being called the first time, for recursive calls to process.nextTick it'll be called as soon as the process.nextTick queue is empty.

You'll note that we had discussed using things besides setTimeout, but the reason it was vetod for 2 reasons, the first is because the process module gets included in a LOT of code so any bloat on our end would be passed into a staggering number of down stream codebases which is something we wanted to avoid. The second and more subtle reason is that this code get's run in a dizzying array of environments (and if you don't believe me look at the issues related to caching setTimeout) so we would have to deal with a lot of very strange corner cases that could break code even for some users that don't even use process.nextTick.

That being said you could always check out my library immediate which is literally just process.nextTick but also using mutation observers and message channels and whatnot it's probably exactly what you want if the default one is not quite doing it for you.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[feross]

We need process.nextTick for consistent async behavior. Read https://blog.izs.me/2013/08/designing-apis-for-asynchrony for more information.

@jimmywarting What do you think about swapping all uses of process.nextTick with immediate to resolve this issue?

[jimmywarting]

We need process.nextTick for consistent async behavior. Read https://blog.izs.me/2013/08/designing-apis-for-asynchrony for more information.

Isaac has a point that a callback should always be either async or sync for consistent behavior. but i would not follow he's rule so strictly, if you don't need it to be consistent and you can handle both scenarios then why bother using next tick or immediate at all? In my fork i just removed it, seems to work just fine.

I would follow that rule more strictly if it is a one time thing but if it is something that is repeatable and slows down performance under lots of load then i would thing twice before using it. especially when it will be executed in the background.

One reason for not using a next cycle would be b/c of the indexed-db layer.
if you can insert/read stuff in a same transaction everything would just work much faster to store seeded files and export everything when you want to save something.

[feross]

if you don't need it to be consistent and you can handle both scenarios then why bother using next tick or immediate at all? In my fork i just removed it, seems to work just fine.

It's about making the API work correctly even when the user of it doesn't code defensively. It prevents really nasty tricky bugs when the user assumes that the function will always be called asynchronously.

[feross]

I assume this issue title was meant to say "background" not "foreground", right? I updated the title.

[feross]

The solution that I'd like to propose here is to switch all process.nextTick calls across the codebase to queueMicrotask. This is a new API analogous to Node's process.nextTick but it works in the browser environment too.

I published a package to make it easier to use: https://github.com/feross/queue-microtask

[feross]

If folks would like to start sending PRs to replace usage of setTimeout(…, 0) and process.nextTick(…), I've sent a few to simple-peer and simple-websocket already, which you can use as examples:

• feross/simple-peer#543
• feross/simple-websocket#59

[KayleePop]

Here's a PR for immediate-chunk-store feross/immediate-chunk-store#9 (merged)

BTW, this problem goes away when async functions are used instead of callbacks

[feross]

@KayleePop Thanks for the PR :)

@KayleePop BTW, this problem goes away when async functions are used instead of callbacks

Yep. I'd love to switch to a promise-based API at some point :)