Fix Connection pool is full, discarding connection

[chemelli74]

Close response object explicitly to avoid logging a warning.

Simone

[kbr]

That's strange. Both parts of the code consume the response.content. According to the requests source code on consumed content the urllib3 should release the connection to the pool again (same on the urllib3 documentation). Therefore the note in the requests documentation that .close normally should not get called explicitly.

Are there a lot of threads or instances of fritzconnection working in parallel?

[chemelli74]

Are there a lot of threads or instances of fritzconnection working in parallel?

Issue with pools comes up when we do a few more call_action calls. For example for users with more than 3/4 port forward rules or call deflections rules.

Simone

[chemelli74]

This is maybe a better approach: https://stackoverflow.com/questions/18466079/can-i-change-the-connection-pool-size-for-pythons-requests-module

Simone

[kbr]

This is maybe a better approach: https://stackoverflow.com/questions/18466079/can-i-change-the-connection-pool-size-for-pythons-requests-module

Yes, I think so too. As far as I understand it urllib3 is handling more connections than can get managed by the pool. That's nothing bad, but a drawback in speed. Seems to be that the mentioned home-assistant application is doing too much in parallel than can get handled by the urllib3 default pool-size.

[chemelli74]

This is maybe a better approach: https://stackoverflow.com/questions/18466079/can-i-change-the-connection-pool-size-for-pythons-requests-module

Yes, I think so too. As far as I understand it urllib3 is handling more connections than can get managed by the pool. That's nothing bad, but a drawback in speed. Seems to be that the mentioned home-assistant application is doing too much in parallel than can get handled by the urllib3 default pool-size.

Let me test it then ,and update this PR if all is fine.

Simone

[chemelli74]

Works fine 👍

HTTPAdapter default adapter is "pool_connections=10, pool_maxsize=10, max_retries=0, pool_block=False"

Added a parameter to init FritzConnection to allow external projects to change this default.

Simone

[kbr]

There is much more work to do, to introduce dynamic pool settings. I have created a new branch dev/connection_pool for this.

[chemelli74]

There is much more work to do, to introduce dynamic pool settings. I have created a new branch dev/connection_pool for this.

Fine for me. I was just reading around and using this method with 20 I cannot reproduce anymore any issue on my HA installation.

Simone

[chemelli74]

There is much more work to do, to introduce dynamic pool settings.

Can you point me to some documentation ? I cannot find anything about dynamic pool. I would like to contribute.

I have created a new branch dev/connection_pool for this.

Is that still only local ?

Simone

[kbr]

Sorry, the branch dev/connection_pool is now available again.

Can you point me to some documentation ? I cannot find anything about dynamic pool. I would like to contribute.

Dynamic pool settings may be a bit misleading. What I mean is the option to set the pool-size for a session. Beside your PR there is a bit more work to do to integrate this feature. But it is a start. However I would like to refactor the code a little bit like:

# same defaults as used by requests:
DEFAULT_POOL_CONNECTIONS = 10
DEFAULT_POOL_MAXSIZE = 10

# supported protocols:
PROTOCOLS = ['http://', 'https://']

class FritzConnection:
    """
    all the updated documentation here ...
    """
    def __init__(
        self,
        address=None,
        port=None,
        user=None,
        password=None,
        timeout=None,
        use_tls=False,
        pool_connections=DEFAULT_POOL_CONNECTIONS,
        pool_maxsize=DEFAULT_POOL_MAXSIZE,
    ):
        """
        all the updated documentation here ...
        """

        ...

        session = requests.Session()
        session.verify = False
        if password:
            session.auth = HTTPDigestAuth(user, password)
        # set pool parameters
        adapter = requests.adapters.HTTPAdapter(
            pool_connections=pool_connections, pool_maxsize=pool_maxsize)
        session.mount(PROTOCOLS[use_tls], adapter)

        ...

[chemelli74]

What I mean is the option to set the pool-size for a session. Beside your PR there is a bit more work to do to integrate this feature. But it is a start.

Place share what's missing and I'll work on it ;-)

However I would like to refactor the code a little bit like:

Good point, will update code.

Simone

[chemelli74]

Thank you for the work – I've put some comments to the code.

I think the branch is ready for a merge in master. What you think ?

Simone

[kbr]

Will have a second look before merging, but this should go to master.

[chemelli74]

Will have a second look before merging, but this should go to master.

I hope you can merge and release a new version of the library. We would really like to bump it on HA and fix a few related issues ;-)

Simone

[kbr]

Is now part of release 1.6.0