Skip to content

feat(core): make inner option of ThrottlingRequestManager optional - #4037

Closed
harryautomazione wants to merge 7 commits into
apify:masterfrom
harryautomazione:feat/throttling-optional-inner
Closed

feat(core): make inner option of ThrottlingRequestManager optional#4037
harryautomazione wants to merge 7 commits into
apify:masterfrom
harryautomazione:feat/throttling-optional-inner

Conversation

@harryautomazione

Copy link
Copy Markdown
Contributor

Summary

Closes #4027

This PR makes the inner option of ThrottlingRequestManagerOptions optional (inner?: T). When omitted, ThrottlingRequestManager automatically opens and defaults to a standard RequestQueue via requestManagerOpener during #ensureSubManagers().

Changes

  • ThrottlingRequestManagerOptions & Schema (packages/core/src/storages/throttling_request_manager.ts):
    • Made inner?: T optional in ThrottlingRequestManagerOptions.
    • Updated Zod validation schema throttlingRequestManagerOptionsSchema with inner: schemas.anyObject.optional().
  • ThrottlingRequestManager:
    • Lazily initializes #inner in #ensureSubManagers() via await this.#requestManagerOpener(null, { configuration: this.config }) if options.inner was omitted.
    • Ensured thread-safety across all aggregation/forwarding methods (isEmpty, purge, #forEachManager, #sumOverManagers, #everyManager).
  • Tests (test/core/storages/throttling_request_manager.test.ts):
    • Added unit test coverage verifying that ThrottlingRequestManager can be instantiated without inner and correctly routes and processes requests.

Checklist

  • Code follows the style guidelines of Crawlee
  • Self-review performed
  • Unit tests added and passing

@janbuchar
janbuchar self-requested a review August 15, 2026 09:32
@B4nan
B4nan changed the base branch from v4 to master August 18, 2026 16:33
@B4nan
B4nan force-pushed the feat/throttling-optional-inner branch from 7b64e7b to 0b30bda Compare August 18, 2026 18:25
@B4nan

B4nan commented Aug 18, 2026

Copy link
Copy Markdown
Member

heads up: the v4 branch was merged into master (v3 development moved to the 3.x branch), so I rebased this branch onto master and force-pushed it to your fork. Your commits are unchanged, just replayed on the new base. Before adding more changes, reset your local branch to the remote one (e.g. git fetch && git reset --hard @{u}), otherwise you'd reintroduce the old history.

@harryautomazione

Copy link
Copy Markdown
Contributor Author

Thanks for the rebase @B4nan, I've synced the branch with master, updated the public API reports and resolved a relative import issue. All CI checks are green now.

@B4nan B4nan changed the title feat(core): make inner option of ThrottlingRequestManager optional (#4027) feat(core): make inner option of ThrottlingRequestManager optional Aug 19, 2026
@janbuchar

Copy link
Copy Markdown
Contributor

Thanks @harryautomazione — and sorry, I've folded this into #4061 rather than asking you to rebase it.

That branch replaced #inner with a lazy factory (inner takes T | (() => T | Promise<T>), resolved once in #getInner()), so the this.#inner! / await this.#ensureSubManagers() pairs here land in code that no longer exists — a trial merge conflicts in 10 places in throttling_request_manager.ts, two of them inside isEmpty() / isFinished(), which are now a single checkReadiness(). On top of that branch the feature is a schema .optional(), inner?:, and one constructor branch defaulting the factory to the default queue, so rebasing this would have meant rewriting it.

Your RequestManagerOpener signature change went in as-is — a defaulted opener needs exactly that identifier?: string | StorageIdentifier | null. The zod (value: number) annotations I left out; master builds clean without them here, so if you are still hitting that error it is worth its own PR.

Closing as superseded by #4061, which closes #4027 with it.

@janbuchar janbuchar closed this Aug 26, 2026
janbuchar added a commit that referenced this pull request Aug 27, 2026
…anager to fix leaky abstraction of ThrottlingRequestManager (#4061)

- closes #3999
- closes #4027
- supersedes #4037
- supersedes #4038
- contributes to #4039

Worth reviewing in order — each change stands alone, with the reasoning
in its own message.

1. `checkReadiness()` replaces `isEmpty()` / `isFinished()` on
`IRequestLoader` and `IRequestManager`.
2. `IRequestManager.recordPacingSignal()` replaces the
`SupportsDomainThrottling` capability probe.
3. The crawler builds the `sameDomainDelaySecs` throttler in its
constructor, which is what puts it *inside* the tandem.
4. `sameDomainDelaySecs` goes in through `recordPacingSignal()` as a
floor covering every domain, so a manager that already paces takes it
instead of getting a second pacer wrapped around it.
5. `ThrottlingRequestManager`'s `inner` is optional — omitted, it opens
the default queue on first use. Same feature as #4037, which is a much
smaller patch on top of the lazy `inner` factory this branch already
has.

On #3999 this takes the first option — wrappers forward — but the
objection there was that it makes `RequestManagerTandem` know throttling
exists. It doesn't: pacing signals are `IRequestManager` members, so the
tandem forwards a `PacingSignal` it never looks inside, and learns
nothing throttling-specific. That deletes the capability-discovery
problem rather than solving it, which is why the delegation interface
from #4038 isn't here.

One capability check did survive that — the guard refusing
`sameDomainDelaySecs` over a `ThrottlingRequestManager` — and it missed
a throttler behind a tandem (#discussion_r3851833671). The fourth change
removes it rather than deepening it: the floor is a `PacingSignal` like
any other, so whatever paces takes it wherever it sits in a composition,
and a manager pacing only *some* of its domains throws rather than
under-applying it. No `instanceof` left anywhere.

#4039 is the shape `sameDomainDelaySecs` was producing on its own:
wrapping happened at first use, by which point the tandem for a
`requestList` already existed, so the pacer went outside it and a list's
requests reached the queue without passing a per-domain clock. The third
commit builds the pacer in the constructor instead, so those requests
are routed by domain like any other. The hand-built form in #4039's
reproduction is untouched — `fetchNextRequest` still takes whatever
`inner` offers without checking the clocks — so #4040 is still needed.

Migration notes in `docs/upgrading/upgrading_v4.md`.
@harryautomazione

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed explanation and for incorporating the opener signature changes into #4061, @janbuchar! Makes complete sense with the new checkReadiness() architecture.

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.

Make the inner option of ThrottlingRequestManager optional

4 participants