fix: Correctly track the number of requests handled by a crawler

[janbuchar]

• closes Remove BasicCrawler.handledRequestsCount #3083
• closes Sharing the RequestManager instance breaks maxRequestsPerCrawl limits #3330
• Technically, making the setter throw an error for handledRequestsCount is breaking, but 1) this is an undocumented internal property and 2) there is only one ancient usage of it in apify-projects

[barjin]

lgtm, thanks!

I'd be more cautious about the breaking change, but I'll leave it up to you.

[barjin]

Side note: the failed requests are technically also "finished" (word-wise, not in Crawlee), I lobby for renaming this to requestsSucceeded in v4.

[B4nan]

My point stands, do we really care for v3? I would postpone for v4, breaking change is a breaking change, there are many other users outside of Apify, so just checking their projects doesn't feel like strong evidence to me.

Or does this block anyone? I thought it was just a random finding of ours, and in such case, I would target v4 to stay on the safe side.

edit: hmm I do see the setter there, so I guess the PR description is not up to date?

[janbuchar]

edit: hmm I do see the setter there, so I guess the PR description is not up to date?

Just a poor choice of words, sorry. The setter is there, but it throws an error.

My point stands, do we really care for v3? I would postpone for v4,
Or does this block anyone? I thought it was just a random finding of ours, and in such case, I would target v4 to stay on the safe side.

Its related to the same logic as #3153, which should probably be fixed sooner than v4. And I'd like to be able to keep "we're fixing those maxCrawlPages issues in v3" as a ground truth in my head, instead of making reality more complicated than that.

breaking change is a breaking change,

Fixing a bug is a breaking change, renaming BasicCrawler is a breaking change as well. But it'd be quite a stretch to call these equal. This is removing an undocumented property accessible only by inheritance from a class not particularly well suited for being extended by end users.

there are many other users outside of Apify, so just checking their projects doesn't feel like strong evidence to me.

Fair enough, but it is a signal, albeit a weak one.

If you think strongly about this, I'm not opposed to rebasing the PR onto v4, but I believe that the breaking-ness of this change is closer to the "making stuff work as originally intended" end of the spectrum.