capacity/limit is misdocumented

aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python

https://docs.aiohttp.org

Other

15.12k stars 2.02k forks source link

capacity/limit is misdocumented #1624

Closed ExplodingCabbage closed 7 years ago

ExplodingCabbage commented 7 years ago

client_reference.rst:

[ ] Still documents the old limit parameter, not the new capacity one
[ ] Lists the default value for all the connector subclasses as None, rather than 20.

I'd PR a fix, but as I noted in https://github.com/KeepSafe/aiohttp/issues/1601#issuecomment-279258716 I disagree with @fafhrd91's change to the limit/capacity functionality so I'll wait for that argument to be resolved first - no sense documenting something that I'm currently arguing you guys should revert!

fafhrd91 commented 7 years ago

i agree with your reasoning, but i still think we need capacity feature.

i suggest rename capacity to max_connections and add max_connections_per_host

asvetlov commented 7 years ago

Let don't invite very_long_names_in_java_style. This is documentation issue. capacity is good name, if we document limit as obsolete and deprecated the meaning of attribute is obvious. limit is also perfect name but already used.

@ExplodingCabbage regarding to your specific case (web scrapper throttling) -- please use explicit semaphores or something like this. Or, maybe, create a ClientSession instance per site.

aiohttp should have perfect defaults for generic cases. I believe it means that ClientSession should behave like browser. Most users don't care about connection limits but they are very sensitive to memory/resource leaks.

fafhrd91 commented 7 years ago

i think we should support per host limit. only reason is, limit per host it is not that trivial to implement.

asvetlov commented 7 years ago

good point. Then let's choose capacity and capacity_per_host as alias for deprecated limit. They are shorter than max_connections and add max_connections_per_host. I believe total limit is much more wider used than limit per host.

fafhrd91 commented 7 years ago

but maybe we should just leave limit in place, just change it's meaning. and add limit_per_host for old limit meaning.

fafhrd91 commented 7 years ago

i renamed capacity back to limit and added limit_per_host

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

If you feel like there's important points made in this discussion, please include those exceprts into that new issue.