Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: Throat causes a halt in execution under certain conditions #61

Closed
bjornua opened this issue Sep 6, 2021 · 4 comments
Closed

bug: Throat causes a halt in execution under certain conditions #61

bjornua opened this issue Sep 6, 2021 · 4 comments
Labels

Comments

@bjornua
Copy link

bjornua commented Sep 6, 2021

Describe the bug

Throat causes a halt in execution under certain conditions

To Reproduce

const _ = require('lodash');
const throat = require('throat');

async function main() {
  const worker = throat(4, async function (page) {});
  const pagesChunks = _.range(10).map((n) => _.range(100));

  let awaitCount = 0;
  for (const pagesChunk of pagesChunks) {
    console.log(`Await ${awaitCount++}`);
    await Promise.all(pagesChunk.map(worker));
  }

  console.log('Done');
}

main();

This outputs:

Await 0
Await 1
Await 2

Expected behaviour

Expected output:

Await 0
Await 1
Await 2
Await 3
Await 4
Await 5
Await 6
Await 7
Await 8
Await 9
Done

Versions

  • node version: v14.17.5
  • npm/yarn version: 6.14.14
  • package version: throat-6.0.1

Additional context

Messing around with the length of pagesChunk or size can make the script complete normally, or halt earlier or later in the iterations.

@bjornua bjornua added the bug label Sep 6, 2021
zkx5xkt pushed a commit to zkx5xkt/throat that referenced this issue Nov 9, 2021
@andriimaryskevych
Copy link

We faced the same issue with this library. @ForbesLindesay please take a look at this - the library has a critical bug and it has 18m+ weekly downloads

@peterlundberg
Copy link

peterlundberg commented May 12, 2022

Can confirm and isolate this a bit more also without the chunking above.

And yes the blocksize is the magic number 64 https://github.com/ForbesLindesay/throat/blob/master/index.js#L105

async function demoExitBug() {
  const limitedConcurrency = throat(1);
  for (let i = 0; i < 100; i += 1) {
    console.log('working on', i);
    await Promise.all([
      limitedConcurrency(async () => true), // with just one it would complete
      limitedConcurrency(async () => true), // concurrency + 1 = only 64 ok
      // limitedConcurrency(async () => true), // concurrency + 2 = only 32 ok
    ]);
  }
  console.log('Done!'); // This will not be reached in 6.0.1
}
demoExitBug();

Looks like this was introduced in 6.0.1 8fd7a02#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R123

joshkel added a commit to joshkel/jest that referenced this issue Jun 23, 2022
Throat has a significant unacknowledged bug (ForbesLindesay/throat#61) that may cause it to lock up with repeated invocations.

Sindre's p-limit appears to be better supported.  I picked p-limit 3.x (the last version before the switch to pure ESM) to match Jest's usage of other packages from Sindre.

Fixes jestjs#12757
@ForbesLindesay
Copy link
Owner

Sorry, didn't notice the issue. Pinging me on twitter is often good for super high priority issues like this. I'll push out a fix shortly.

@ForbesLindesay
Copy link
Owner

This is resolved in v6.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants