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

Resolving more than 'X' URLs with capture-website, lacks concurrency for multiple promises #3

Closed
tannerdolby opened this issue May 13, 2021 · 6 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@tannerdolby
Copy link
Owner

tannerdolby commented May 13, 2021

Since the capture-website function is returning a Promise, when there are 10 are more URLs that are attempting to have screenshots taken, the following error occurs:

(node:2793) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 exit listeners added. Use emitter.setMaxListeners() to increase limit
(node:2793) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGINT listeners added. Use emitter.setMaxListeners() to increase limit
(node:2793) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGTERM listeners added. Use emitter.setMaxListeners() to increase limit
(node:2793) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGHUP listeners added. Use emitter.setMaxListeners() to increase limit

I found the exact same issue over in capture-website, and they recommend using p-map.

That looks promising because indeed I have 26 blog post articles in a collection that uses the post.njk layout, so at build time there are 26 templates being created for each post that uses the post.njk layout. Thus, 26 inputs are being promised so 26 Promise<void> are being returned. Using p-map to map over promises concurrently should solve the issue.

@tannerdolby tannerdolby added the enhancement New feature or request label May 13, 2021
@tannerdolby
Copy link
Owner Author

tannerdolby commented May 13, 2021

Resolved by using limited concurrency using p-limit. I tried using p-map but kept getting an error about it not liking import { AggregateErrors, maybe because its CommonJS by default in the Node.js eleventy config file. Mixing CommonJS and ES6 modules must be partly to blame for the p-map not working. Currently the implementation with p-limit is working.

const limit = pLimit(4); // allow 4 promises at once
promiseArr.push(limit(() => capture(bool, config))) 
// where capture(bool, config) returns Promise<void> from captureWebsite

Promise.all(promiseArr).then(res => {
  // image generated
}).catch(err => throw err))

@tannerdolby
Copy link
Owner Author

Still not fixed, its not limiting correctly. For the number of posts in my collection.posts the length is 18, 16 of those are being used in the captureWebsite function.

I need to batch the captureWebsite calls because the limit() isnt limiting concurrency or I'm not implementing correctly. Images are being created and the plugin "is functional" but need to handle this concurrency issue.

@tannerdolby
Copy link
Owner Author

(node) warning: possible EventEmitter memory leak detected. 11 a listeners added. Use emitter.setMaxListeners() to increase limit.

The above is referring to a possible EventEmitter memory leak, a leak occurs when you continuously add event handlers without removing them. This particularly happens when you use a single emitter instance numerous times.

You've continuously added handlers to error and end, but have not removed them, even if data was successfully read and those handlers are no longer relevant.

@tannerdolby
Copy link
Owner Author

I have a bunch of functions from captureWebsite being invoked and listening to the same event emitter. :-(

@tannerdolby
Copy link
Owner Author

tannerdolby commented May 14, 2021

I'm launching chrome processes in parallel. Every chrome instance adds a listener to the process's "exit" event to cleanup properly. 12 chrome instances add 12 listeners, which yields the warning.

You can fix this by adding process.setMaxListeners(Infinity) in the very beginning of your script:

@tannerdolby tannerdolby added the bug Something isn't working label May 14, 2021
@tannerdolby
Copy link
Owner Author

Closing for a more succinct issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant