Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

async context manager and test #2333

Conversation

nathan-chappell
Copy link

Signed-off-by: Nathan Chappell nchappell@mono-software.com

Enhancement / Feature

Currently, we would like to use the python-vcx bindings in a server environment. The server must operate in an async-hronous fashion, and in some cases needs to use libvcx. While spawning a new process to completely isolate all libvcx operations would be ideal, it is unfortunately not an option for us. Since libvcx can only "manage one connection at a time" (see comment below), we require synchronization between async processes. This has led to the need to use the idiom:

await synchronization mechanism
await libvcx initialization
# ... do work
libvcx shutdown
release synchronization mechanism

This seems like the type of thing that should be a part of the libraries underlying utilities, here are a few reasons why:

  • it's an obvious pattern whose logic can be consolidated
  • the library implementers can add or remove any additional logic that is required / deprecated in the future
  • it documents / communicates the semantics for multiple-uses of the library at the same time

Comments

I'm no domain-expert, nor do I have a lot of experience with this library. The phrasing "manage one connection at a time" may be erroneous, but the intent should be clear. Also, it may be that additional logic is required for these context managers to be fully functional, we're exploring options with our current implementation.

Signed-off-by: Nathan Chappell <nchappell@mono-software.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 22, 2020

This pull request introduces 1 alert when merging 7d223f5 into 7f4fbb0 - view on LGTM.com

new alerts:

  • 1 for Unused import

Signed-off-by: Nathan Chappell <nchappell@mono-software.com>
@nathan-chappell
Copy link
Author

I don't know why jenkins failed - none of my code touched anything else. I did notice an issue while running tests though - the python tests for get_version were throwing SEGFAULTS and killing everything. The implementation seems to use ctypes improperly and dereferences an invalid pointer. If that's the case, I can put in a correction to the python code, but I don't know what the desired behavior of get_version should be...

Currently, there's a bug in the implementation. While the rust-api for get_version indicates that a *const c_char is returned from a call to vcx_version, the python wrapper do_call_sync automatically assumes every return value from the library is an integer. This leads to problems when reporting errors, and eventually leads to a SEGFAULT when the python-function get_version tries to cast the integer into a c_char_p. The restype needs to be specified before calling the foreign function - see this stack overflow post for details.

At any rate, I could fix this, but I would just make the do_call and do_call_sync functions behave differently depending on the return type of the rust library... Maybe there is a better way, but that's the most direct way.

@WadeBarnes WadeBarnes added the wont-address-project-depreciated This Issue or PR will not be addressed. The project has been depreciated. label Oct 24, 2023
@ryjones ryjones closed this Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wont-address-project-depreciated This Issue or PR will not be addressed. The project has been depreciated.
Development

Successfully merging this pull request may close these issues.

4 participants