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

Support python 3.11 #628

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Support python 3.11 #628

merged 2 commits into from
Oct 27, 2022

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Oct 27, 2022

In the changelog, this is called out as "official support" since the code all worked on 3.11 prior to this change.

Places touched:

  • CI testing job on linux (all pythons)
  • CI testing on windows (latest python only)
  • CI testing on macos (latest python only)
  • readthedocs config
  • pypi classifiers
  • tox testenvs

CI builds should be examined to see what might fail in particular. Local testing under pyenv+tox works, but it's always possible that something will be different in the CI environment.

In the changelog, this is called out as "official support" since the
code all worked on 3.11 prior to this change.

Places touched:
- CI testing job on linux (all pythons)
- CI testing on windows (latest python only)
- CI testing on macos (latest python only)
- readthedocs config
- pypi classifiers
- tox testenvs

CI builds should be examined to see what might fail in particular.
Local testing under pyenv+tox works, but it's always possible that
something will be different in the CI environment.
Copy link
Member

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I see the Windows CI is encountering that most wonderful of all issues, "the shutil.rmtree() that fails for no discernible reason". In the past I "fixed" this by sleeping for .1 seconds in an except clause, at which point trying again would succeed.

That kind of fix is only acceptable in a production environment, though, so you might just try re-running the test or confirming that everything touching that sqlite file has released / closed. 😉

@sirosen
Copy link
Member Author

sirosen commented Oct 27, 2022

I think I'm going to explicitly close the sqlite connections created by the tests. I'm somewhat surprised this hasn't been an issue before now -- we're opening those connections and then trusting GC to clean them up. I might even want to add a close() method to the storage adapter if we don't have one already.

This is being done to fix the behavior of tests which are leaking
resources. The leaked connections can cause tests to fail because
there's still a handle on the `.db` file when the temp dir tree is
being removed.

The method makes it possible to close the connection through a public
interface. Rather than adding a context manager interface, for now the
testsuite operates by having a fixture which auto-closes SQLiteAdapter
instances during test teardown.
@sirosen
Copy link
Member Author

sirosen commented Oct 27, 2022

Whew! That was more of a rewrite to the tests than I expected, but I've just added SQLiteAdapter.close() and made sure tests call it. 🤞 that this fixes this build and makes the tests more stable in the long run.

Copy link
Member

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

On Linux (and probably Mac), opening a file for read/write doesn't prevent the file from being deleted. Try this:

# Terminal 1
f = open("abc", "w+")

# Terminal 2
rm abc

# Back to terminal 1
f.write("not backed by the original file anymore? no problem!")
f.close()

# And in terminal 2...
ls abc  # finds nothing!

On Windows, though, an open file cannot be deleted so you're depending on garbage collection. Note that PyPy and CPython garbage collect differently, so it's best to explicitly close resources always! 🚪 🥳

@sirosen
Copy link
Member Author

sirosen commented Oct 27, 2022

Yeah, IMO this was a resource leak and we should probably double-check SQLiteAdapter usage sites.

It makes me want to add a context manager interface which closes on __exit__ as well, as a future change.

@sirosen sirosen merged commit 41b4f75 into globus:main Oct 27, 2022
@sirosen sirosen deleted the support-py311 branch October 27, 2022 20:36
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.

2 participants