fix: prevent DevToolsServer port conflicts on BrowserPool relaunch#212
fix: prevent DevToolsServer port conflicts on BrowserPool relaunch#212nikitachapovskii-dev merged 3 commits intomasterfrom
Conversation
|
It's interesting why this moved to Backlog... ( |
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
Thank you @nikitachapovskii-dev !
I only have a few points, mostly regarding the naming:
| if (!this._devToolsExitHookRegistered) { | ||
| this._devToolsExitHookRegistered = true; | ||
| Actor.on('exit', () => { | ||
| this._devToolsServer?.stop?.(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
nit: this guard (_devToolsExitHookRegistered) is imo not necessary, as this IIFE (L236 and below) will only run exactly once, no?
There was a problem hiding this comment.
Yes, for our current case it isn’t necessary. It's just an extra safety guard to prevent accidentally registering multiple Actor.on('exit') listeners if (whatever?) causes rerunning the startup block. Assuming that IIFE always runs only once we can remove this
| private static _devToolsServer: any | null = null; | ||
| private static _devToolsExitHookRegistered = false; | ||
|
|
||
| private static async _startDevToolsServerOnce(): Promise<void> { |
There was a problem hiding this comment.
| private static async _startDevToolsServerOnce(): Promise<void> { | |
| private static async startDevToolsServerOnce(): Promise<void> { |
private is enough, eslint is right :) We try not to do underscores in our new TS code (there are some legacy exceptions).
| chromeRemoteDebuggingPort: CHROME_DEBUGGER_PORT, | ||
| }); | ||
|
|
||
| await this._devToolsServer.start(); |
There was a problem hiding this comment.
nit: this._devToolsServer doesn't have to be a static variable, we do not refer to it anywhere else.
What if _startDevToolsServerOnce (maybe renamed to sth like getDevToolsServer()) returned Promise<Server> instead, so the caller can do as they see fit - wdyt?
There was a problem hiding this comment.
Agreed, we don't need that as a static.
Return of the started server instance seems good for me.
| @@ -1,5 +1,15 @@ | |||
| import { launchPuppeteer } from '@crawlee/puppeteer'; | |||
| import type { Browser, Page } from 'puppeteer'; | |||
| import { | |||
There was a problem hiding this comment.
I believe this test file was made to test the bundle.browser.ts file (see the name). Please update the name or create a new test file.
3 participants
Fixes a Web Scraper dev-mode issue where DevToolsServer was started from BrowserPool’s preLaunchHook on every browser launch/relaunch, causing EADDRINUSE port conflicts after browser/page crashes or pool relaunches. The PR makes DevToolsServer startup idempotent by starting it only once per actor process (reusing a cached start promise on subsequent hook calls) and registers a single Actor.on('exit') cleanup to stop the server.
Closes #208