Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions src/crawlee/http_clients/_httpx.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ def _build_request(
method=method,
headers=dict(headers) if headers else None,
content=payload,
cookies=session.cookies.jar if session else None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this fixes the direct-request case, but httpx still drops session cookies on redirect. Client._redirect_headers ends with an unconditional headers.pop("Cookie", None) and rebuilds cookies from the client jar, which crawlee deliberately keeps empty (_HttpxTransport deletes Set-Cookie), so per-request cookies= never survives a hop.

Measured against a local /start -> 302 -> /echo with this branch: impit (both persistence modes) and curl forward auth=secret, httpx sends no Cookie at all. Not a regression, but it is the same defect this PR targets and auth flows redirect constantly. Either set the Cookie header explicitly (reusing _get_cookie_header, as the impit path now does) or seed the client jar per request. At minimum, add a redirect case to the shared cookie tests -- the new tests hide this, since test_send_request_sends_session_cookies hits /cookies directly and test_persist_cookies_per_session_* only asserts jar contents.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: with cookies= added here, _build_request and the inline request construction in crawl (L163-171) are now equivalent apart from the timeout expression, so crawl can just call self._build_request(...). Two hand-maintained copies of this wiring is exactly how the original cookies= asymmetry crept in.

extensions={'crawlee_session': session if self._persist_cookies_per_session else None},
timeout=timeout or httpx.USE_CLIENT_DEFAULT,
)
Expand Down Expand Up @@ -333,15 +334,19 @@ def _get_client(self, proxy_url: str | None) -> httpx.AsyncClient:
def _combine_headers(self, explicit_headers: HttpHeaders | None) -> HttpHeaders | None:
"""Merge default headers with explicit headers for an HTTP request.

Generate a final set of request headers by combining default headers, a random User-Agent header,
and any explicitly provided headers.
Generate a final set of request headers by combining default headers from a single fingerprint
(Accept, Accept-Language, User-Agent) and any explicitly provided headers. Using one fingerprint
avoids mixing Accept headers from one browser profile with a User-Agent from another.
"""
common_headers = self._header_generator.get_common_headers() if self._header_generator else HttpHeaders()
user_agent_header = (
self._header_generator.get_random_user_agent_header() if self._header_generator else HttpHeaders()
)
if self._header_generator:
generated_headers = self._header_generator.get_specific_headers(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this was the last production caller of HeaderGenerator.get_common_headers and get_random_user_agent_header (fingerprint_suite/_header_generator.py:48,57) -- only tests/unit/fingerprint_suite/test_header_generator.py still exercises them. They are public API so removal is out of scope here, but worth marking deprecated rather than leaving as unexplained dead weight.

header_names={'Accept', 'Accept-Language', 'User-Agent'},
)
else:
generated_headers = HttpHeaders()

explicit_headers = explicit_headers or HttpHeaders()
headers = common_headers | user_agent_header | explicit_headers
headers = generated_headers | explicit_headers
return headers or None

@staticmethod
Expand Down
85 changes: 66 additions & 19 deletions src/crawlee/http_clients/_impit.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from contextlib import asynccontextmanager
from logging import getLogger
from typing import TYPE_CHECKING, Any, TypedDict
from urllib.request import Request as UrllibRequest

from cachetools import LRUCache
from impit import AsyncClient, Browser, HTTPError, Response, TimeoutException, TransportError
Expand All @@ -30,6 +31,9 @@

logger = getLogger(__name__)

# Cache key: (proxy_url, id(cookie_jar) or None)
_ClientCacheKey = tuple[str | None, int | None]
Comment on lines +34 to +35

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: http.cookiejar.CookieJar overrides neither __eq__ nor __hash__, so it already hashes by identity -- (proxy_url, cookie_jar) is the exact key you want. _ClientCacheKey, _make_cache_key (L269), the identity recheck (L282) and the _ClientCacheEntry TypedDict all exist only to route around id(). Lifetime is unchanged either way, since the entry already held a strong ref to the jar. It needs CookieJar imported at runtime rather than under TYPE_CHECKING (stdlib, cheap), after which LRUCache[tuple[str | None, CookieJar | None], AsyncClient] drops the alias, this comment, the helper and the TypedDict in one edit -- and _get_client goes back to its pre-PR shape.



class _ClientCacheEntry(TypedDict):
"""Type definition for client cache entries."""
Expand Down Expand Up @@ -116,7 +120,42 @@ def __init__(

self._async_client_kwargs = async_client_kwargs

self._client_by_proxy_url = LRUCache[str | None, _ClientCacheEntry](maxsize=10)
self._client_cache = LRUCache[_ClientCacheKey, _ClientCacheEntry](maxsize=10)

def _prepare_cookies_and_headers(
self,
*,
session: Session | None,
url: str,
headers: HttpHeaders | dict[str, str] | None,
) -> tuple[CookieJar | None, HttpHeaders]:
"""Resolve cookie jar / Cookie header based on `persist_cookies_per_session`.

When persistence is enabled, attach the session jar to Impit so response cookies update it.
When persistence is disabled, send existing cookies via the `Cookie` header and keep the
shared client (no jar) so clients stay cached and reusable.
Comment on lines +132 to +136

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these lines stop at ~96-99 chars against the 120-col limit the rest of the file fills, and the codebase writes impit rather than "Impit".

Suggested change
"""Resolve cookie jar / Cookie header based on `persist_cookies_per_session`.
When persistence is enabled, attach the session jar to Impit so response cookies update it.
When persistence is disabled, send existing cookies via the `Cookie` header and keep the
shared client (no jar) so clients stay cached and reusable.
"""Resolve cookie jar / Cookie header based on `persist_cookies_per_session`.
When persistence is enabled, attach the session jar to `impit` so response cookies update it. When
persistence is disabled, send existing cookies via the `Cookie` header and keep the shared client (no jar)
so clients stay cached and reusable.

"""
if isinstance(headers, dict) or headers is None:
headers = HttpHeaders(headers or {})

if session is None:
return None, headers

if self._persist_cookies_per_session:
return session.cookies.jar, headers

if cookie_header := self._get_cookie_header(session.cookies.jar, url, headers):
headers = headers | HttpHeaders({'Cookie': cookie_header})

return None, headers

@staticmethod
def _get_cookie_header(jar: CookieJar, url: str, headers: HttpHeaders | None = None) -> str:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the only caller always passes headers, so the default is dead.

Suggested change
def _get_cookie_header(jar: CookieJar, url: str, headers: HttpHeaders | None = None) -> str:
def _get_cookie_header(jar: CookieJar, url: str, headers: HttpHeaders) -> str:

"""Build a Cookie request header from a jar without attaching the jar to the client."""
# UrllibRequest is only used to format Cookie headers via CookieJar; it never opens a connection.
request = UrllibRequest(url, headers=dict(headers) if headers else {}) # noqa: S310
jar.add_cookie_header(request)
return request.get_header('Cookie', '')

@override
async def crawl(
Expand All @@ -128,14 +167,19 @@ async def crawl(
statistics: Statistics | None = None,
timeout: timedelta | None = None,
) -> HttpCrawlingResult:
client = self._get_client(proxy_info.url if proxy_info else None, session.cookies.jar if session else None)
cookie_jar, headers = self._prepare_cookies_and_headers(
session=session,
url=request.url,
headers=request.headers,
)
client = self._get_client(proxy_info.url if proxy_info else None, cookie_jar)

try:
response = await client.request(
url=request.url,
method=request.method,
content=request.payload,
headers=dict(request.headers) if request.headers else None,
headers=dict(headers) if headers else None,
timeout=timeout.total_seconds() if timeout else None,
)
except TimeoutException as exc:
Expand Down Expand Up @@ -166,10 +210,8 @@ async def send_request(
) -> HttpResponse:
validate_http_url(url)

if isinstance(headers, dict) or headers is None:
headers = HttpHeaders(headers or {})

client = self._get_client(proxy_info.url if proxy_info else None, session.cookies.jar if session else None)
cookie_jar, headers = self._prepare_cookies_and_headers(session=session, url=url, headers=headers)
client = self._get_client(proxy_info.url if proxy_info else None, cookie_jar)

try:
response = await client.request(
Expand Down Expand Up @@ -203,7 +245,8 @@ async def stream(
) -> AsyncGenerator[HttpResponse]:
validate_http_url(url)

client = self._get_client(proxy_info.url if proxy_info else None, session.cookies.jar if session else None)
cookie_jar, headers = self._prepare_cookies_and_headers(session=session, url=url, headers=headers)
client = self._get_client(proxy_info.url if proxy_info else None, cookie_jar)

try:
response = await client.request(
Expand All @@ -222,18 +265,22 @@ async def stream(
finally:
response.close()

@staticmethod
def _make_cache_key(proxy_url: str | None, cookie_jar: CookieJar | None) -> _ClientCacheKey:
return (proxy_url, id(cookie_jar) if cookie_jar is not None else None)
Comment on lines +269 to +270

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a static helper with a single call site that builds a two-element tuple -- inline it, or drop it entirely (see the comment on L34).


def _get_client(self, proxy_url: str | None, cookie_jar: CookieJar | None) -> AsyncClient:
"""Retrieve or create an HTTP client for the given proxy URL.
"""Retrieve or create an HTTP client for the given proxy URL and cookie jar.

If a client for the specified proxy URL does not exist, create and store a new one.
Clients are cached by `(proxy_url, cookie_jar identity)` so sessions with different jars do not share
a client. When cookie persistence is disabled, cookies are sent via headers and `cookie_jar` is `None`,
so a shared client can be reused for the proxy.
"""
cached_data = self._client_by_proxy_url.get(proxy_url)
if cached_data:
client = cached_data['client']
client_cookie_jar = cached_data['cookie_jar']
if client_cookie_jar is cookie_jar:
# If the cookie jar matches, return the existing client.
return client
cache_key = self._make_cache_key(proxy_url, cookie_jar)

cached_data = self._client_cache.get(cache_key)
if cached_data and cached_data['cookie_jar'] is cookie_jar:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: cached_data['cookie_jar'] is cookie_jar can never be False -- the key already contains id(cookie_jar) and the entry keeps that jar alive, so a live key cannot map to a different object. (That is also why the id()-reuse hazard is genuinely safe here, just unnecessary.)

return cached_data['client']

# Prepare a default kwargs for the new client.
kwargs: dict[str, Any] = {
Expand All @@ -249,7 +296,7 @@ def _get_client(self, proxy_url: str | None, cookie_jar: CookieJar | None) -> As

client = AsyncClient(**kwargs, cookie_jar=cookie_jar)

self._client_by_proxy_url[proxy_url] = _ClientCacheEntry(client=client, cookie_jar=cookie_jar)
self._client_cache[cache_key] = _ClientCacheEntry(client=client, cookie_jar=cookie_jar)

return client

Expand All @@ -270,4 +317,4 @@ def _is_proxy_error(error: HTTPError) -> bool:
@override
async def cleanup(self) -> None:
"""Clean up resources used by the HTTP client."""
self._client_by_proxy_url.clear()
self._client_cache.clear()
64 changes: 64 additions & 0 deletions tests/unit/http_clients/test_http_clients.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import asyncio
import importlib
import json
import os
import sys
from typing import TYPE_CHECKING
Expand All @@ -14,6 +15,7 @@
from crawlee import Request
from crawlee.errors import ProxyError
from crawlee.http_clients import CurlImpersonateHttpClient, HttpClient, HttpxHttpClient, ImpitHttpClient
from crawlee.sessions import Session
from crawlee.statistics import Statistics
from tests.unit.server import generate_file_content
from tests.unit.server_endpoints import HELLO_WORLD
Expand Down Expand Up @@ -323,3 +325,65 @@ def test_import_error_handled(optional_module_name: str, import_path: str) -> No
sys.modules.pop(mod_name, None)
with pytest.raises(ImportError):
importlib.import_module(import_path)


async def test_send_request_sends_session_cookies(http_client: HttpClient, server_url: URL) -> None:
"""`send_request` must attach existing session cookies (same as `crawl`)."""
session = Session()
session.cookies.set('auth', 'token-1', domain=server_url.host or '127.0.0.1', path='/')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: or '127.0.0.1' is unreachable for the local test server -- it is only narrowing URL.host from str | None. Same on L344. Fine to leave, but a single narrowed local would say that more clearly than a fallback that looks like real behaviour.


response = await http_client.send_request(str(server_url / 'cookies'), session=session)
body = json.loads(await response.read())

assert body['cookies'] == {'auth': 'token-1'}


async def test_stream_sends_session_cookies(http_client: HttpClient, server_url: URL) -> None:
"""`stream` must attach existing session cookies (same as `crawl`)."""
session = Session()
session.cookies.set('auth', 'token-2', domain=server_url.host or '127.0.0.1', path='/')

content = b''
async with http_client.stream(str(server_url / 'cookies'), session=session) as response:
async for chunk in response.read_stream():
content += chunk

assert json.loads(content)['cookies'] == {'auth': 'token-2'}


@pytest.mark.parametrize(
'custom_http_client',
[
pytest.param(CurlImpersonateHttpClient(persist_cookies_per_session=False), id='curl'),
pytest.param(HttpxHttpClient(persist_cookies_per_session=False), id='httpx'),
pytest.param(ImpitHttpClient(persist_cookies_per_session=False), id='impit'),
],
indirect=['custom_http_client'],
)
async def test_persist_cookies_per_session_false(custom_http_client: HttpClient, server_url: URL) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: no test enters the new Cookie-header branch at _impit.py:147, which is the whole point of the impit change. This test starts from a fresh Session() with an empty jar, so _get_cookie_header returns '' and the branch is skipped; test_send_request_sends_session_cookies uses the default clients (persist=True) and takes the jar path instead. I confirmed the branch works, but nothing here would catch it breaking -- one parametrized case with ImpitHttpClient(persist_cookies_per_session=False) plus a pre-seeded session, asserting /cookies echoes the cookies, closes it.

"""When persistence is disabled, response Set-Cookie must not update the session jar."""
session = Session()
request = Request.from_url(str(server_url.with_path('set_cookies').extend_query(a=1)))

await custom_http_client.crawl(request, session=session)

assert {cookie['name']: cookie['value'] for cookie in session.cookies.get_cookies_as_dicts()} == {}


@pytest.mark.parametrize(
'custom_http_client',
[
pytest.param(CurlImpersonateHttpClient(persist_cookies_per_session=True), id='curl'),
pytest.param(HttpxHttpClient(persist_cookies_per_session=True), id='httpx'),
pytest.param(ImpitHttpClient(persist_cookies_per_session=True), id='impit'),
],
indirect=['custom_http_client'],
)
async def test_persist_cookies_per_session_true(custom_http_client: HttpClient, server_url: URL) -> None:
"""When persistence is enabled, response Set-Cookie must update the session jar."""
session = Session()
request = Request.from_url(str(server_url.with_path('set_cookies').extend_query(a=1)))

await custom_http_client.crawl(request, session=session)

assert {cookie['name']: cookie['value'] for cookie in session.cookies.get_cookies_as_dicts()} == {'a': '1'}
19 changes: 19 additions & 0 deletions tests/unit/http_clients/test_httpx.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
import json
import logging
from typing import TYPE_CHECKING
from unittest.mock import patch

import pytest

from crawlee._types import HttpHeaders
from crawlee.fingerprint_suite import HeaderGenerator
from crawlee.fingerprint_suite._browserforge_adapter import get_available_header_values
from crawlee.fingerprint_suite._consts import COMMON_ACCEPT_LANGUAGE
from crawlee.http_clients import HttpxHttpClient
Expand Down Expand Up @@ -54,3 +57,19 @@ async def test_common_headers_and_user_agent(server_url: URL, header_network: di
assert 'user-agent' in response_headers
assert 'python-httpx' not in response_headers['user-agent']
assert response_headers['user-agent'] in get_available_header_values(header_network, {'User-Agent', 'user-agent'})


async def test_headers_come_from_single_fingerprint() -> None:
"""Accept and User-Agent must come from the same generated fingerprint profile."""
header_generator = HeaderGenerator()
fingerprint = {'Accept': 'text/html', 'Accept-Language': 'en-US', 'User-Agent': 'TestAgent/1.0'}

with patch.object(header_generator, 'get_specific_headers', return_value=HttpHeaders(fingerprint)) as mocked:
client = HttpxHttpClient(header_generator=header_generator)
combined = client._combine_headers(None)

mocked.assert_called_once_with(header_names={'Accept', 'Accept-Language', 'User-Agent'})
Comment on lines +64 to +71

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this test cannot fail for the reason it exists. It patches get_specific_headers to return a fixed dict, then asserts that dict came back -- but the property being claimed (Accept and User-Agent from one fingerprint) lives one level down, in how many times BrowserforgeHeaderGenerator.generate runs. Re-introducing a separate generate() call for the User-Agent would leave this green. Patch header_generator._generator.generate instead, assert exactly one call, and assert all three headers trace to that one dict. Two side notes: instantiating a real HeaderGenerator() loads the whole browserforge network just to have its method replaced (a Mock suffices), and project style prefers monkeypatch/Mock over unittest.mock.patch.

assert combined is not None
assert combined['accept'] == 'text/html'
assert combined['accept-language'] == 'en-US'
assert combined['user-agent'] == 'TestAgent/1.0'
25 changes: 25 additions & 0 deletions tests/unit/http_clients/test_impit.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from __future__ import annotations

from typing import TYPE_CHECKING

from crawlee.http_clients import ImpitHttpClient

if TYPE_CHECKING:
from yarl import URL


async def test_cleanup_clears_client_cache(server_url: URL) -> None:
"""`ImpitHttpClient.cleanup` must drop cached clients so the next request creates a fresh one."""
client = ImpitHttpClient()
async with client:
await client.send_request(str(server_url))
assert len(client._client_cache) == 1
first_client = next(iter(client._client_cache.values()))['client']

await client.cleanup()
assert len(client._client_cache) == 0

await client.send_request(str(server_url))
assert len(client._client_cache) == 1
second_client = next(iter(client._client_cache.values()))['client']
assert second_client is not first_client
Loading