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

fix(httpx): don't save response if filtered to None #909

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alekseik1
Copy link
Contributor

VCR py fails to record response made by starlette (and FastAPI as well) TestClient #628.
It seems like the problem hides in this assert that is triggered by this func call.

I took a quick look and it looks like we're trying to record response to cassete object here.
But at the same time, here we see that whenever filters render response to None, a real request is always performed. So, why should we record real response if we don't use it anyway? I made a small check with should_save_response variable that runs same filters and skips recording if it will never be replayed.

There's one catch though. Image following scenario:

  1. User sets ignore_hosts=["some.real.host.org"], runs tests and gets his cassettes.
  2. Neither of cassettes contains requests to "some.real.host.org" since we skipped writing them (by should_save_response var).
  3. Now, user decides to set ignore_hosts=[] and rerun his tests.
  4. His cassettes are no longer valid because of a missing request to "some.real.host.org". Such behavior is introduced by this PR.

Does it look sensible?

@alekseik1 alekseik1 force-pushed the fix/dont-save-filtered-response-for-httpx branch from 713cded to f002c31 Compare January 27, 2025 14:57
@alekseik1
Copy link
Contributor Author

damn, this anyio stuff is really hard to debug. By trials and errors, I found out that run_coroutine_threadsafe seem to fix problems with TestClient. My idea is that starlette's TestClient might lead to race conditions when reading response because TestClient uses threads internally (see here).

Unfortunately, any attempt to reproduce bug in test environment failed - all tests were green without my fix applied. I believe that problems with TestClient start on large codebase, when FastAPI has lots of Tasks/Futures running in handles.

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

Successfully merging this pull request may close these issues.

1 participant