Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add a close statement to ensure session is closed on error #1777

Merged

Conversation

KimEbert42
Copy link
Contributor

Askar, on error, would end up not returning connections to the connection pool because aca-py left the connections open.

@ianco
Copy link
Contributor

ianco commented May 18, 2022

One of the tests is failing:

aries_cloudagent/indy/credx/tests/test_cred_issuance.py:222: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
aries_cloudagent/indy/credx/issuer.py:177: in create_and_store_credential_definition
    await txn.commit()
aries_cloudagent/core/profile.py:204: in __aexit__
    await self._teardown()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <[AttributeError("'NoneType' object has no attribute 'is_transaction'",) raised in repr()] AskarProfileSession object at 0x7fe728b97da0>
commit = None

    async def _teardown(self, commit: bool = None):
        """Dispose of the session or transaction connection."""
        if commit:
            try:
                await self._handle.commit()
            except AskarError as err:
                raise ProfileError("Error committing transaction") from err
>       await self._handle.close()
E       AttributeError: 'NoneType' object has no attribute 'close'

aries_cloudagent/askar/profile.py:216: AttributeError

@andrewwhitehead
Copy link
Contributor

Are you able to test the changes in this PR? #1692

The store should still be closed in the case of an exception when it is garbage collected, however there might be an existing reference preventing that from happening. The garbage collector should still handle circular references so I'm not sure how it would accumulate many of these, though.

Does the error still happen with Sqlite as a backend? There was a potentially related fix in sqlx, but it doesn't look like it's being backported to a 0.5.x release which is unfortunate. launchbadge/sqlx#1799

@KimEbert42
Copy link
Contributor Author

@andrewwhitehead I tested against #1692, and that doesn't solve the problem. It appears the issue may be related to the Python garbage collector not closing the session quickly enough, leaving connection pool objects borrowed from the pool. To keep the pool operating efficiently, we should probably close the connection as quickly as possible.

@KimEbert42
Copy link
Contributor Author

@ianco fixed the tests.

@ianco
Copy link
Contributor

ianco commented May 19, 2022

@reflectivedevelopment if you sync up wth the main branch then we can merge

@ianco ianco enabled auto-merge May 19, 2022 23:05
@ianco ianco merged commit ce11560 into openwallet-foundation:main May 20, 2022
@TheTechmage TheTechmage deleted the fix/fix-askar-close-bug branch May 26, 2022 13:32
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.

3 participants