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 problems with python 3.9 #140

Merged
merged 3 commits into from
Apr 14, 2022
Merged

Fix problems with python 3.9 #140

merged 3 commits into from
Apr 14, 2022

Conversation

rtobar
Copy link
Contributor

@rtobar rtobar commented Apr 13, 2022

This fixes the main problems seen with python 3.9, and adds it back to the Travis build matrix.

There are still some stability problems with the unit tests around the multiprocessing support (one test runs out of file descriptors, and another leaves a dangling subprocess), but they are both currently circumvented (by chance!) in Travis, and they can also be easily avoided locally, so I'll leave those out for the time being as they require more work.

We have been getting away with this code, but in reality we have always
been guilty of not taking the GIL in the "pythonic" dynlibs (i.e., those
offering a "init2" method rather than an "init" one). Until 3.8 no
errors have showed up, but in 3.9 segfaults start to happen when calling
PyLong_FromLong (and possibly other methods).

To solve this I've updated our example code to take the GIL as
appropriate. This is verbose, but it's the simples solution. A different
approach would have been to load the dynlib with ctypes's PyDLL rather
than CDLL, but that forces all C function calls to hold the GIL even if
not required, which is less performant. Nobody (except Kevin, if he
still does?) is using this anyway, and if so they would have run into
the same error as we did here, so there's no issue in clarifying that
the GIL must be taken by the dynlib code rather than by daliuge.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
This was found while stabilising the unit tests running against python
3.9, which in turn I was doing to have a stable basis to work against
for YAN-968.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
Now that the problems with 3.9 have been solved we should be able to
test against it without problems.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar rtobar requested a review from awicenec April 13, 2022 07:59
@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.863% when pulling 96693a0 on fix-3.9-crashes into 7ba6073 on master.

@coveralls
Copy link

coveralls commented Apr 13, 2022

Coverage Status

Coverage increased (+13.9%) to 80.734% when pulling 96693a0 on fix-3.9-crashes into 7ba6073 on master.

Copy link
Contributor

@awicenec awicenec left a comment

Choose a reason for hiding this comment

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

Interesting fixes with the GIL. Make sure that the additional multiprocessing requirements are captured for future upgrades.

@rtobar rtobar closed this Apr 14, 2022
@rtobar rtobar deleted the fix-3.9-crashes branch April 14, 2022 01:16
@rtobar rtobar merged commit 96693a0 into master Apr 14, 2022
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