search

apify / browser-pool

A Node.js library to easily manage and rotate a pool of web browsers, using any of the popular browser automation libraries like Puppeteer, Playwright, or SecretAgent.
87 stars 14 forks source link

refactor: use cancellable timeouts via `@apify/timeout` #62

Closed B4nan closed 2 years ago

B4nan commented 2 years ago

This is needed to deal with the timeouts in SDK. We do call tryCancel() even outside of the timeout handler - that is to allow early escape from the SDK functions that use the browser pool (and are wrapped inside addTimeoutToPromise call). The tryCancel() call outside of such handler is a no-op, so it is safe even if the pool would be used elsewhere.

Related: https://github.com/apify/apify-js/issues/1102 https://github.com/apify/apify-js/issues/1216

B4nan commented 2 years ago

No tests?

Nothing really changed, and existing tests are passing, so it should be enough. I will try to come up with some tests for the SDK integration. But more importantly, seems like the timeouting issue is not present when running via jest :/ Initially I tried to write a test case and things just magically worked (the task promise got cancelled after test finished, so once the timeout passed). When I ran it via node as a script directly, the issue was present 🤷 So it seems like it is impossible to write a test that would fail with the problem this is trying to solve, at least via jest.

B4nan commented 2 years ago

Added test to demonstrate how this can be useful from SDK, also added tests to the SDK PR.