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: task.__del__ to use _saved_name #95

Merged
merged 1 commit into from
May 12, 2020

Conversation

rares-pop
Copy link

My understanding is that when this is getting garbage collected, the python environment is shutting down and importing the _lib fails when trying to get the 'name' property. Basically the sys.meta_path is empty by the time is trying to load the daqmx dll.

Using the already saved name will prevent loading the dll on del.

Signed-off-by: Rares POP rares.pop@ni.com

Signed-off-by: Rares POP <rares.pop@ni.com>
@rares-pop rares-pop linked an issue May 11, 2020 that may be closed by this pull request
@rares-pop
Copy link
Author

@spalatofhi - can you take a look at this?

@rares-pop rares-pop self-assigned this May 11, 2020
@spalatofhi
Copy link

I haven't tested. I think this should fix the issue, because _saved_name is set inside __init__ (indirectly). A better fix, perhaps, would be to avoid the import inside check_for_error.

@rares-pop
Copy link
Author

rares-pop commented May 11, 2020

Strangely enough, the _saved_name is there for close() - but I don't understand yet the reason if any. For __del__ it works just nice.

It would be nice to cache the _lib and use that everywhere, as opposed to load it each time. But this is the pattern that is used right now everywhere and I don't want to break it in one instance. Maybe a larger refactor can fix it.

I did test this on Windows 10 64bit (py 3.6.3) and LinuxRT (py3.5), and works as expected.

This sort of thing didn't get triggered by the tests though, due to how pytest works and the fact that this wasn't the only test.

@rares-pop rares-pop merged commit 55b162c into ni:master May 12, 2020
@spalatofhi
Copy link

Hi,
The import in check_for_error seems to be a workaround to avoid a circular import. The "proper" fix is probably to restructure the error modules. The import is still performed exactly once, so the _lib is thus always the same instance.

As of python 3.8 there is now a builtin for caching properties:
https://docs.python.org/3/library/functools.html#functools.cached_property
Similar tools for memoization have been around for longer.

Thanks for fixing this.

@rares-pop
Copy link
Author

@spalatofhi - I did create this - #99

I'm part of the new team that is going to own this repository, and until we are figuring out the level of investment (the time we have allocated to work on it), I'm tackling things I believe I can make progress on without a huge ramp-up.

Thank you for the contributions you are making and helping us get this straight!

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.

Task.__del__ broken
2 participants