Skip to content

bpo-44895: Temporarily add an extra gc.collect() call #27746

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 13, 2021
Merged

bpo-44895: Temporarily add an extra gc.collect() call #27746

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Lib/test/test_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,6 +1014,9 @@ def cycle():

def test_no_hang_on_context_chain_cycle2(self):
# See issue 25782. Cycle at head of context chain.
Copy link
Member

Choose a reason for hiding this comment

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

This fix looks incorrect, tests should not depend on the GC to pass. When this happens, is a symptom of another problem.

I propose to revert this commit and investigate.

Copy link
Member

Choose a reason for hiding this comment

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

CC: @vstinner

Copy link
Member

Choose a reason for hiding this comment

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

I understand is a "temporary measure" but in my experience those are left there with no fixes more often than not. Also, I don't feel comfortable with "
temporary fixes in the release candidate.

Copy link
Member Author

Choose a reason for hiding this comment

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

The alternative is to disable the test. That doesn't fix the issue either.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to deactivate the test. The reason is that relying on the GC in this way at the end has global effects and can mask other issues. Is also not deterministic and can actually be an endless loop in some extreme situations involving resurrection.

This is just my opinion on this of course, If the consensus is to leave this because the test has more value, then let's leave it, but I have to say that my previous experience with these kind of fixes is that they are left there more often than not.

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry, is not urgent.

Thanks a lot for the investigation and for all the work!! 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel comfortable with " temporary fixes in the release candidate.

Sure, we weren't going to let this slip into RC2. The point was to make refleak tests able to catch other regressions on that branch in the mean time.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you'd rather redo the fix as a skip instead of gc.collect() then that's fine as well. However, from what I understood on the PR, having it run on the entire buildbot fleet for a few days would give us more confidence whether that approach to working around the refleaks is even effective.

How about we leave it as is for the weekend and remove the gc.collect() loop on Monday?

Copy link
Member

Choose a reason for hiding this comment

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

If you'd rather redo the fix as a skip instead of gc.collect() then that's fine as well. However, from what I understood on the PR, having it run on the entire buildbot fleet for a few days would give us more confidence whether that approach to working around the refleaks is even effective.

I don't get what you mean by this. Why do we want to know if the approach to work around is effective? What information do we gain by this? I can understand the though that this may gives us some more light into the problem but this workaround is too intrusive to gather any conclusions from the actual problem, more then that a cycle is likely involved.

How about we leave it as is for the weekend and remove the gc.collect() loop on Monday?

👍 Works for me

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to know if the approach to work around is effective?

Irit wrote:

Let's run this on buildbots and if it indeed resolves the leak we can do this for now.

I misinterpreted this as "let's merge this and see" but obviously she meant the test-with-buildbots label. Nevermind!

while gc.collect():
# Remove this once issue 44895 is resolved
pass

class A(Exception):
pass
Expand Down