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

[BUG] 'Request context disposed' was reported when use route.fetch() #23781

Closed
erma0 opened this issue Jun 18, 2023 · 11 comments
Closed

[BUG] 'Request context disposed' was reported when use route.fetch() #23781

erma0 opened this issue Jun 18, 2023 · 11 comments
Assignees
Labels

Comments

@erma0
Copy link

erma0 commented Jun 18, 2023

Context:

  • Playwright Version: 1.35.0
  • Operating System: Windows 11
  • Python version: 3.11.2
  • Browser: Chromium-msedge
  • Extra:

Code Snippet

from playwright.sync_api import Route, sync_playwright


def handle(route: Route):
    res = route.fetch()
    route.fulfill(response=res)


with sync_playwright() as playwright:
    browser = playwright.chromium.launch(channel='msedge', headless=False)
    context = browser.new_context()
    page = context.new_page()

    page.route("**/*", handle)
    page.goto("https://www.baidu.com/", wait_until='domcontentloaded')
    page.unroute("**/*") # not work

    page.close()
    context.close()
    browser.close()

Describe the bug
'Request context disposed' or 'Response context disposed'

I think it's because the page has been closed, but the route handle function is still running.

It appears to be related to microsoft/playwright-python#1402 microsoft/playwright-python#1433 microsoft/playwright-python#1458 and has been fixed, but this issue still persists.

How can I stop a running route handle function before the page is closed?

thanks.

@mxschmitt
Copy link
Member

Reproducible with Node.js, moving upstream so it can be discussed there.

import { chromium } from 'playwright';

(async () => {
  const browser = await chromium.launch({
    headless: false
  });
  const context = await browser.newContext();
  const page = await context.newPage();
  await page.route('**/*', async route => {
    const response = await route.fetch();
    route.fulfill({
      response
    });
  });
  await page.goto('https://www.baidu.com/', {
    waitUntil: 'domcontentloaded'
  });
  // ---------------------
  await context.close();
  await browser.close();
})();

@mxschmitt mxschmitt transferred this issue from microsoft/playwright-python Jun 19, 2023
@dgozman dgozman self-assigned this Jun 20, 2023
@dgozman dgozman added the v1.36 label Jun 20, 2023
@dgozman
Copy link
Contributor

dgozman commented Jun 29, 2023

Investigation notes:

  • Playwright should not silently catch errors in user code to avoid masking problems.
  • fetch() must throw when request cannot be made, because it returns a value.

Possible improvements, that could still lead to minor regression in existing test suites:

  • unroute() to await all ongoing route handlers that it removes.
  • page.close()/context.close() to automatically call unrouteAll() that would await ongoing handlers.
    Both improvements do not help with pages that were closed by the browser, for example with popupWindow.close().

@daelmaak
Copy link

This is still a problem, and I didn't find a solution to this except for not doing route.fetch() at all. Any update on this?

@nbaldzhiev
Copy link

I am getting Error: apiRequestContext.fetch: Request context disposed. errors, but in Webkit (playwright 1.38.0).
A related discussion I found is https://github.com/microsoft/playwright/discussions/12810.
The suggestion there is to add a page.waitForResponse call, but it doesn't always help in our case as we still see this failure from time to time.

Our code (redacted):

  await page.route(url, async route => {
    const response = await page.request.fetch(route.request())
    const body = await response.json()
    do-some-modifications-to-body
    route.fulfill({
      response,
      body: JSON.stringify(body)
    })
  })

  await page.waitForResponse(url, { timeout: 15 * 1000 })

@frakic
Copy link

frakic commented Oct 11, 2023

A related discussion I found is https://github.com/microsoft/playwright/discussions/12810. The suggestion there is to add a page.waitForResponse call, but it doesn't always help in our case as we still see this failure from time to time.

The link's broken.

I'm getting this error on @playwright/test version 1.38.1, NodeJS v16.20.2 when trying to use context.request.fetch().

For some reason, I only experience this issue when I run my tests against an instance of my React app that is being run locally.

If I test against a deployed version of my React app, the command executes without a problem. I've even tried building the React app and serving it locally, but the issue persists. Kind of weird...

Anyway, I hope this gets fixed soon because it's slowing me down so much.

@btomaj
Copy link

btomaj commented Oct 28, 2023

I can confirm that neither page.waitForResponse() nor page.waitForLoadState("load" | "domcontentloaded" | "networkidle") solve this problem.

In my scenario, none of page, context, or browser are closed; my tests conclude before requests have been processed by page.route() in a fixture, causing subsequent tests to fail. This seems like a serious design flaw: the test causing the problem passes, causing the next test to fail when page.route() throws an error from the previous test.

The proposed solution to await all ongoing route handlers that unroute() removes doesn't solve for my scenario. Calling page.unroute() after await use() in a fixture throws the following error:

Error: page.unroute: Target closed

What's required to move forward and implement a fix?

@ovistoica
Copy link

Bump this. We have the same issue

@dgozman dgozman assigned yury-s and unassigned dgozman Nov 3, 2023
@dgozman
Copy link
Contributor

dgozman commented Nov 3, 2023

Capturing notes from the team discussion


  1. pyee proposal:
  • when event handler throws/rejects, we catch
  • we emit 'error' with the exception
  1. hybrid proposal:
    • when event handler throws/rejects, we catch
  • we emit 'error' with the exception when the page is closed
  • we throw if the page is still open
  1. "waiting" proposal:
  • when we call page.close() or context.close(), unregister all handlers and wait for them
  • then actually close
  • opt-in or opt-out
  1. "explicit api" proposal:
  • page.route() gets a single option to 1) opt-out of waiting and simultaneously 2) opt-in into silent catch after unroute/close is called
  • page.unroute() unregsiters and waits for ongoing ones, gets an option to opt-out of waiting
  • page.close() and context.close() call page.unroute('**/*')
  • later on, similar page.onAsync() with a similar option?

So far, the proposal 4 is the preferred one.

@btomaj
Copy link

btomaj commented Nov 8, 2023

@dgozman agree with you that proposal 4 is preferred. Is that what you moved forward with?

@yury-s
Copy link
Member

yury-s commented Nov 29, 2023

Discussion notes:

unroute

  • we remove routes right away and do not call user callback anymore
  • we await ongoing user callbacks unless opted out
  • the invariant is no handler calls after unroute has finished

close

  • we do not call user callbacks anymore
  • we await ongoing user callbacks unless opted out
  • one of the two:
    • we stall all requests that match routing patterns
    • we stall all requests from the page
  • we cannot simply remove all handlers and disable network interception as it may result in network requests which wouldn't happen before (hermetic test).

runBeforeUnload: true

All bets are off as it may actually result in not closing the page. No special logic for this scenario. It's still possible to call unrouteAll() manually before closing the page if needed.

@yury-s yury-s closed this as completed Nov 29, 2023
@yury-s yury-s reopened this Nov 29, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Nov 30, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Nov 30, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Nov 30, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Dec 1, 2023
yury-s added a commit that referenced this issue Dec 4, 2023
…#28484)

Previously we were wrongly firing `route` event for the request which
are not in fact intercepted (e.g. requests from service worker).

Related #28414
Reference #23781
yury-s added a commit to yury-s/playwright that referenced this issue Dec 4, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Dec 14, 2023
yury-s added a commit that referenced this issue Dec 14, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Dec 15, 2023
yury-s added a commit that referenced this issue Dec 15, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Dec 15, 2023
@yury-s
Copy link
Member

yury-s commented Dec 15, 2023

We ended up with a new unrouteAll() method which should help with errors like this. It will be released in 1.41

@yury-s yury-s closed this as completed Dec 15, 2023
yury-s added a commit to yury-s/playwright that referenced this issue Dec 15, 2023
yury-s added a commit that referenced this issue Dec 15, 2023
This option does not make sense in the synchronous API where all active
routes would be on the same call stack any way.

Reference #23781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants