Skip to content

Commit

Permalink
feat!: close BiDi session properly
Browse files Browse the repository at this point in the history
`browser.disconnect` returns `Promise<void>` instead of `void`. This is required ti be able to properly close BiDi session before disconnecting.
  • Loading branch information
Maksim Sadym committed Nov 30, 2023
1 parent 0e6852d commit b54333f
Show file tree
Hide file tree
Showing 11 changed files with 32 additions and 21 deletions.
4 changes: 2 additions & 2 deletions docs/api/puppeteer.browser.disconnect.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ Disconnects Puppeteer from this [browser](./puppeteer.browser.md), but leaves th

```typescript
class Browser {
abstract disconnect(): void;
abstract disconnect(): Promise<void>;
}
```

**Returns:**

void
Promise&lt;void&gt;
2 changes: 1 addition & 1 deletion docs/api/puppeteer.browser.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const browser = await puppeteer.launch();
// Store the endpoint to be able to reconnect to the browser.
const browserWSEndpoint = browser.wsEndpoint();
// Disconnect puppeteer from the browser.
browser.disconnect();
await browser.disconnect();

// Use the endpoint to reestablish a connection
const browser2 = await puppeteer.connect({browserWSEndpoint});
Expand Down
4 changes: 2 additions & 2 deletions packages/puppeteer-core/src/api/Browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ export interface BrowserEvents extends Record<EventType, unknown> {
* // Store the endpoint to be able to reconnect to the browser.
* const browserWSEndpoint = browser.wsEndpoint();
* // Disconnect puppeteer from the browser.
* browser.disconnect();
* await browser.disconnect();
*
* // Use the endpoint to reestablish a connection
* const browser2 = await puppeteer.connect({browserWSEndpoint});
Expand Down Expand Up @@ -414,7 +414,7 @@ export abstract class Browser extends EventEmitter<BrowserEvents> {
* Disconnects Puppeteer from this {@link Browser | browser}, but leaves the
* process running.
*/
abstract disconnect(): void;
abstract disconnect(): Promise<void>;

/**
* Whether Puppeteer is connected to this {@link Browser | browser}.
Expand Down
10 changes: 8 additions & 2 deletions packages/puppeteer-core/src/bidi/Browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,13 @@ export class BidiBrowser extends Browser {
return this.#browserTarget;
}

override disconnect(): void {
this;
override async disconnect(): Promise<void> {
try {
// Fail silently if the session cannot be ended.
await this.#connection.send('session.end', {});
} catch (e) {
debugError(e);
}
this.#connection.dispose();
}
}
4 changes: 4 additions & 0 deletions packages/puppeteer-core/src/bidi/Connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ export interface Commands {
returnType: Bidi.EmptyResult;
};

'session.end': {
params: Bidi.EmptyParams;
returnType: Bidi.EmptyResult;
};
'session.new': {
params: Bidi.Session.NewParameters;
returnType: Bidi.Session.NewResult;
Expand Down
5 changes: 3 additions & 2 deletions packages/puppeteer-core/src/cdp/Browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,13 +410,14 @@ export class CdpBrowser extends BrowserBase {

override async close(): Promise<void> {
await this.#closeCallback.call(null);
this.disconnect();
await this.disconnect();
}

override disconnect(): void {
override disconnect(): Promise<void> {
this.#targetManager.dispose();
this.#connection.dispose();
this._detach();
return Promise.resolve();
}

override get connected(): boolean {
Expand Down
4 changes: 2 additions & 2 deletions test/src/browser.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('Browser specs', function () {
protocol: browser.protocol,
});
expect(remoteBrowser.process()).toBe(null);
remoteBrowser.disconnect();
await remoteBrowser.disconnect();
});
});

Expand All @@ -84,7 +84,7 @@ describe('Browser specs', function () {
protocol: browser.protocol,
});
expect(newBrowser.isConnected()).toBe(true);
newBrowser.disconnect();
await newBrowser.disconnect();
expect(newBrowser.isConnected()).toBe(false);
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/src/browsercontext.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ describe('BrowserContext', function () {
});
const contexts = remoteBrowser.browserContexts();
expect(contexts).toHaveLength(2);
remoteBrowser.disconnect();
await remoteBrowser.disconnect();
await context.close();
});

Expand Down
4 changes: 2 additions & 2 deletions test/src/chromiumonly.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Chromium-Specific Launcher tests', function () {
return 7 * 8;
})
).toBe(56);
browser1.disconnect();
await browser1.disconnect();

const browser2 = await puppeteer.connect({
browserURL: browserURL + '/',
Expand All @@ -49,7 +49,7 @@ describe('Chromium-Specific Launcher tests', function () {
return 8 * 7;
})
).toBe(56);
browser2.disconnect();
await browser2.disconnect();
} finally {
await close();
}
Expand Down
12 changes: 6 additions & 6 deletions test/src/launcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('Launcher specs', function () {
return error_;
});
await server.waitForRequest('/one-style.css');
remote.disconnect();
await remote.disconnect();
const error = await navigationPromise;
expect(
[
Expand All @@ -78,7 +78,7 @@ describe('Launcher specs', function () {
.catch(error_ => {
return error_;
});
remote.disconnect();
await remote.disconnect();
const error = await watchdog;
expect(error.message).toContain('Session closed.');
} finally {
Expand Down Expand Up @@ -635,7 +635,7 @@ describe('Launcher specs', function () {
return 7 * 8;
})
).toBe(56);
otherBrowser.disconnect();
await otherBrowser.disconnect();

const secondPage = await browser.newPage();
expect(
Expand Down Expand Up @@ -776,7 +776,7 @@ describe('Launcher specs', function () {

await page2.close();
await page1.close();
remoteBrowser.disconnect();
await remoteBrowser.disconnect();
await browser.close();
} finally {
await close();
Expand All @@ -788,7 +788,7 @@ describe('Launcher specs', function () {
const browserWSEndpoint = browser.wsEndpoint();
const page = await browser.newPage();
await page.goto(server.PREFIX + '/frames/nested-frames.html');
browser.disconnect();
await browser.disconnect();

const remoteBrowser = await puppeteer.connect({
browserWSEndpoint,
Expand Down Expand Up @@ -858,7 +858,7 @@ describe('Launcher specs', function () {
const browserWSEndpoint = browserOne.wsEndpoint();
const pageOne = await browserOne.newPage();
await pageOne.goto(server.EMPTY_PAGE);
browserOne.disconnect();
await browserOne.disconnect();

const browserTwo = await puppeteer.connect({
browserWSEndpoint,
Expand Down
2 changes: 1 addition & 1 deletion test/src/oopif.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ describe('OOPIF', function () {
return target.url().endsWith('dynamic-oopif.html');
});
await target.page();
browser1.disconnect();
await browser1.disconnect();
});

it('should support lazy OOP frames', async () => {
Expand Down

0 comments on commit b54333f

Please sign in to comment.