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

Open askar sessions only on demand - Connections #1424

Conversation

acuderman
Copy link
Contributor

This PR fixes the connections part of the #1417 issue.

@@ -49,6 +49,11 @@ def bind_providers(self):
),
)

def _init_context(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this is necessary, it's already performed for the session when that is created?

Copy link
Contributor Author

@acuderman acuderman Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is performed on the session only when test_session function is called. But in this case we call test_profile.

Another option would be to call init_context every time we create a new session (in constructor) but I think used approach is clearer since in non test environment, dependencies are also bound to the profile context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The session must be awaited or used as an async context manager. When this happens the _setup method is called, which calls _init_context on the session. If the session has not been initialized this way then it is not technically 'open' (the in-memory backend is more forgiving about this currently). I think that adding this workaround is just masking the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. We removed _init_context on the profile and created sessions with async context.

@andrewwhitehead
Copy link
Contributor

Thanks! Looks very thorough in general

@acuderman acuderman force-pushed the feat-IDENT-3371-askar-sessions-demand-connections branch from fa9b363 to df4e33e Compare September 30, 2021 11:27
@andrewwhitehead andrewwhitehead merged commit 0706a97 into openwallet-foundation:main Sep 30, 2021
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