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

Stack trace is lost from the point the code uses p-map #34

Open
davidfirst opened this issue Mar 19, 2021 · 7 comments
Open

Stack trace is lost from the point the code uses p-map #34

davidfirst opened this issue Mar 19, 2021 · 7 comments

Comments

@davidfirst
Copy link

davidfirst commented Mar 19, 2021

Both, console.trace and throw new Error don't show the stack-trace beyond the point p-map got called. In other words, the functions that called before pMap, disappear from the stack trace.
As a comparison, the p-map-series doesn't suffer from this issue. It does keep the stack-trace.

See the example below, if you run a function that runs the native Promise.all, the stack trace shows the function name - runPromiseNative. Same if you run the function runPromisePmapSeries. However, try to run runPromisePmap, and you'll see how the stack trace truncated.

I tried node v12.x and v14.x.

const pMap = require('p-map');

const pMapSeries = require('p-map-series');

async function promiseFn() {
  throw new Error('stop')
}

async function runPromiseNative() {
  await Promise.all([promiseFn()]).then(() => { console.log('completed') }).catch((err) => console.error(err));
}

async function runPromisePmap() {
  await pMap([promiseFn], fn => fn()).then(() => { console.log('completed') }).catch((err) => console.error(err));
}

async function runPromisePmapSeries() {
  await pMapSeries([promiseFn], fn => fn()).then(() => { console.log('completed') }).catch((err) => console.error(err));
}

// runPromiseNative();

runPromisePmap();

// runPromisePmapSeries();

Results when running runPromisePmap :

Error: stop
    at promiseFn (/Users/davidfirst/teambit/bit/pmap.js:6:9)
    at /Users/davidfirst/teambit/bit/pmap.js:14:33
    at /Users/davidfirst/teambit/bit/node_modules/p-map/index.js:57:28

Results when running runPromiseNative.

Error: stop
    at promiseFn (/Users/davidfirst/teambit/bit/pmap.js:6:9)
    at runPromiseNative (/Users/davidfirst/teambit/bit/pmap.js:10:22)
    at Object.<anonymous> (/Users/davidfirst/teambit/bit/pmap.js:21:1)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47
@sindresorhus
Copy link
Owner

This is, unfortunately, a limitation of JavaScript: https://stackoverflow.com/questions/56644392/passing-an-async-function-as-a-callback-causes-the-error-stack-trace-to-be-lost

The only way to solve this is to:

  1. Find a way to refactor p-map to not use new Promise(). I think the stack trace is preserved if all the async is chained.
  2. Or get the stack trace before the await, then afterward, merge the stack traces somehow. We did something like this in Got: https://github.com/sindresorhus/got/blob/462bc630015064fa4ad4358cf28d24f95e1c958b/source/core/index.ts#L1225-L1237

@sindresorhus sindresorhus changed the title stack trace is lost from the point the code uses p-map Stack trace is lost from the point the code uses p-map Mar 20, 2021
@davidfirst
Copy link
Author

davidfirst commented Mar 21, 2021

Thanks @sindresorhus .
I came up with a different implementation that doesn't involve the new Promise as you suggested. It seems to be working, and the stack trace is fully kept, but the stopOnError is not implemented.
For my use-case that's good enough, but obviously I can't create a PR with this. I'm pasting it here just in case someone will find it useful.

module.exports = async (
	iterable,
	mapper,
	{
		concurrency = Infinity,
		stopOnError = true
	} = {}
) => {
	const results = [];
	const iterator = iterable[Symbol.iterator]();
	let completed = false;
	const runBatch = async () => {
		const items = [];
		for (let i = 0; i < concurrency; i++) {
			const iterableResult = iterator.next();
			if (iterableResult.done) {
				completed = true;
				break;
			}
			items.push(iterableResult.value);
		}
		const batchResults = await Promise.all(items.map(item => mapper(item)));
		results.push(...batchResults);
		if (!completed) await runBatch();
	};
	await runBatch();

	return results;
}

TS version of the above.

export async function pMapPool<T, X>(iterable: T[], mapper: (item: T) => Promise<X>, { concurrency = Infinity } = {}) {
  const results: X[] = [];
  const iterator = iterable[Symbol.iterator]();
  let completed = false;
  const runBatch = async () => {
    const items: T[] = [];
    for (let i = 0; i < concurrency; i += 1) {
      const iterableResult = iterator.next();
      if (iterableResult.done) {
        completed = true;
        break;
      }
      items.push(iterableResult.value);
    }
    const batchResults = await Promise.all(items.map((item) => mapper(item)));
    results.push(...batchResults);
    if (!completed) await runBatch();
  };
  await runBatch();

  return results;
}

@marchuffnagle
Copy link

I looked into this a little bit this morning. Running @davidfirst's examples, I'm able to see the short and long stacktraces. From @sindresorhus's comment, I would have expected that the problem was being caused because we were catching the error on line 59, then passing it to reject.

I created a simplified version of the pMap function to try to replicate this, but I get a full stack trace instead:

const fakepmap = async (f) => {
  return new Promise((resolve, reject) => {

    const next = () => {
      (async () => {
        try {
          resolve(await f());
        } catch (error) {
          reject(error);
        }
      })();
    };

    next();
  });
}

const promiseFn = async () => {
  throw new Error('stop');
};

fakepmap(promiseFn).then(console.log).catch(console.log);

// Error: stop
//     at promiseFn (/Users/me/Source/test.js:19:9)
//     at /Users/me/Source/test.js:7:25
//     at next (/Users/me/Source/test.js:11:9)
//     at /Users/me/Source/test.js:14:5
//     at new Promise (<anonymous>)
//     at fakepmap (/Users/me/Source/test.js:2:10)
//     at Object.<anonymous> (/Users/me/Source/test.js:22:1)
//     at Module._compile (internal/modules/cjs/loader.js:1138:30)
//     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
//     at Module.load (internal/modules/cjs/loader.js:986:32)

Any idea what is missing from this test scenario but is present in pMap that would cause the different stack output?

@septatrix
Copy link

Any idea what is missing from this test scenario but is present in pMap that would cause the different stack output?

I do not know the exact thing but your implementation does not limit the concurrency, maybe that could be an issue. It would be really great to have proper stack traces but so far I have not encountered anything which properly solves this so I assume your implementation omits some step which loses the stack trace but is required for limiting the concurrency.

You could also take a look at p-limit. Its code is a decent bit shorter but suffers from the same issue. It might be easier to figure out which step exactly loses the stack trace in that implementation.

@davidfirst
Copy link
Author

@septatrix,

but your implementation does not limit the concurrency

Why do you think so?

@septatrix
Copy link

but your implementation does not limit the concurrency

Why do you think so?

My answer was in response to the snippet from @marchuffnagle, not yours

@davidfirst
Copy link
Author

but your implementation does not limit the concurrency

Why do you think so?

My answer was in response to the snippet from @marchuffnagle, not yours

Oh sorry, I missed that.

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

No branches or pull requests

4 participants