-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: do not reinit cached browser session in worker #770
Conversation
1044a00
to
7d6e74e
Compare
@@ -52,15 +52,6 @@ module.exports = class ExistingBrowser extends Browser { | |||
return this; | |||
} | |||
|
|||
async reinit(sessionId, sessionOpts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Данный метод использовался для использования закешированного инстанса браузера. Чтобы его реиспользовать подменялся только sessionId
.
На реиспользование закешированного инстанса уходит примерно 0.1ms
, а на создание нового около 100ms
. Т.е. реиспользование инстанса работает в 1000 раз быстрее, но так как значения не существенные, то какой-то значимой просадки при выполнении тестов не будет. Зато в будущем не будет гемора с поддержкой.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
при этом замена самого session id происходит не всегда
@@ -102,10 +102,6 @@ module.exports = class Browser { | |||
return this.publicAPI.sessionId; | |||
} | |||
|
|||
set sessionId(id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Выгасил возможность переопределять sessionId
. Данный инстанс используется только локально и наружу пользователю не торчит.
markAsBroken() { | ||
this.applyState({ isBroken: true }); | ||
|
||
this._stubCommands(); | ||
} | ||
|
||
quit() { | ||
this.sessionId = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Использовался, чтобы понять можно ли реинициализировать закешированный инстанс браузера
@@ -50,7 +31,6 @@ module.exports = class BrowserPool { | |||
throw error; | |||
} | |||
|
|||
browser.sessionId = sessionId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь подменять sessionId
теперь тоже не нужно.
8d80439
to
7d6e74e
Compare
src/worker/runner/browser-pool.js
Outdated
? _.isNil(browser.sessionId) && browser.version === browserVersion | ||
: _.isNil(browser.sessionId); | ||
}); | ||
let browser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const browser = Browser.create(this._config, browserId, browserVersion, this._emitter);
try {
await browser.init({ sessionId, sessionCaps, sessionOpts }, this._calibrator);
catch (e) {
browser.markAsBroken();
this.freeBrowser(browser);
throw Object.assign(error, { meta: browser.meta });
}
this._emitter.emit(WorkerEvents.NEW_BROWSER, browser.publicAPI, { browserId: browser.id, browserVersion });
return browser;
7d6e74e
to
761dcb8
Compare
What is done?
Stop using cache of browsers sessions in worker. Causes:
se:cdp
capability in response withsessionId
on the end of the url and we don't modify it;