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

Replace xfail with gc.collect in TestSubmodule.test_rename #1767

Merged
merged 2 commits into from Dec 12, 2023

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Dec 12, 2023

In various places in the code, a conditional gc.collect() is used to allow a test to finish its work (and pass). This adds one more such place. It accordingly removes an xfail marking. TestSubmodule.test_rename now passes on all combinations of platform and Python version.

Like the xfail condition, running gc.collect() here is conditional, being done only in the specific situation the PermissionError occurs. Besides that it does not always run even on Windows (only in 3.12 and later), this resembles various other conditional and non-conditional gc.collect calls.

The situation is a bit odd, but I think that's--albeit by a fairly slight margin--more of a reason to convert the xfail into a gc.collect() than to refrain from doing so.

Context

#1745 (under "Which versions should we test?"), and also 82c361e, 0b7ee17, and various other less interesting commits, provide further context about this test and the PermissionError on Windows that does not occur prior to 3.12.

Because PermissionError error message is about another process having the file open, #790 seems relevant. But this does not happen in a call to git.util.rmtree.

One call is enough, now

It had previously appeared to me that two calls to gc.collect were required, but I am unable to reproduce that. It may have been specific to how I was running it on my system at that time. The need for only one call may have been brought about by changes to the code in the mean time, but I have tested that only one call appears required even without the changes in #1765. (Perhaps #1753 contributed? I think most likely not, though.)

This is to clearly establish the failure still occurs (since we do
not have strict=True set). It is in preparation for working around
the problems with a call to gc.collect().

See 82c361e and 0b7ee17 for context.
Like the xfail was, this is conditional, being done only in the
specific situation the PermissionError occurs. Besides that it does
not always run even on Windows (only in 3.12 and later), this
resembles various other conditional and non-conditional gc.collect
calls.

It had previously appeared to me that two calls to gc.collect were
required, but I am unable to reproduce that. It may have been
specific to how I was running it on my system at that time. The
need for only one call may have been brought about by changes to
the code in the mean time, but I have tested that only one call
appears required even without the changes in gitpython-developers#1765.
@EliahKagan EliahKagan marked this pull request as ready for review December 12, 2023 14:53
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

That's a great catch! Who would have thought that gc.collect() is also applicable here.

This probably also hints at the need to eventually fix the way GitPython handles systems resources. After all, it really seems like one would rather want to use…

with git.open(path) as repo:
   pass

…to control the lifetime of all opened resources precisely. Alternatively, GitPython would not be allowed to keep anything open past the lifetime of a method call, but that's not feasible at all. Maybe there is alternatives that I am not seeing.

It's interesting how it's obvious that GitPython was designed in a time when there was no GC, but rather, reference counting with deterministic and immediate calls of destructors.

@Byron Byron merged commit 3ac7e78 into gitpython-developers:main Dec 12, 2023
20 checks passed
@EliahKagan EliahKagan deleted the 312-gc branch December 12, 2023 21:06
@EliahKagan
Copy link
Contributor Author

EliahKagan commented Dec 13, 2023

This sounds like it might be part of making the classes that currently have __del__ methods also be context managers, as discussed in comments in #1669. Though I suppose this usage need not wait for that, with Repo having been a context manager since #555. Should the tests be modified to make greater use of that? I'm not sure it would address the sort of thing that happened here and it's not quite what your code example shows.

It's interesting how it's obvious that GitPython was designed in a time when there was no GC, but rather, reference counting with deterministic and immediate calls of destructors.

I have some thoughts about how the effects of this might be mitigated, to some extent, even for existing code that would not use newly added context managers (or perhaps even before, or separately from, making more classes context managers). But right now these are half-formed ideas that might turn out to be meritless and (as with #1762) there's some other GitPython-related stuff I'd probably want to finish dealing with first. I mention this mainly so I don't forget or, if I do, so that it is more likely to come up again.

@Byron
Copy link
Member

Byron commented Dec 13, 2023

This sounds like it might be part of making the classes that currently have __del__ methods also be context managers, as discussed in comments in #1669. Though I suppose this usage need not wait for that, with Repo having been a context manager since #555. Should the tests be modified to make greater use of that? I'm not sure it would address the sort of thing that happened here and it's not quite what your code example shows.

I left some more ideas in this comment, and based on it I'd say the tests should be kept as is. Should a full-on context-manager approach happen, the tests can be a great indicator to show how hard the migration would be for existing users, and also guide towards making it as painless as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants