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 flaky datetime test #860

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Apr 9, 2020

I think I realised there's a simple workaround for #720

The release pool unsoundness in #756 is all related to reference types - &PyAny, &PyDict.

So I think swapping them out for PyObject and Py<PyDict>, which have different drop semantics, should stop this test biting us all the time on CI.

(Fixes #720)

@kngwyu
Copy link
Member

kngwyu commented Apr 9, 2020

Thank you, but how about locking other threads? As an experiment, I opened #862.

@pganssle
Copy link
Member

pganssle commented Apr 9, 2020

Assuming it works, I like two things about #862's approach over this one:

  1. It has minimal changes to the underlying test structure.
  2. When Release pool unsoundness #756 is fixed, we can simply remove the locks and the datetime test will act as something of a regression test.

That said, I'm not sure that I spent a huge amount of time thinking about whether to use a borrow or an owned reference here, so unless I was much more thoughtful about this than I remember being "minimal changes" amounts to "keeping the arbitrary thing I did when I made this instead of switching to another arbitrary, valid choice."

With regards to point 2 - when we fix #756 we should probably write a dedicated regression test with a much higher chance of triggering the bug, so again this is really only a weak preference.

I think we should probably decide between the two options (assuming they both work) based on what we would do if we didn't have release pool soundness issues. I sorta think we've been moving towards the &PyDict / &PyAny recommendation whenever possible (and thus we should go with #862), but I haven't been able to keep up with all the changes y'all have been making lately, so maybe that's out-of-date information?

@davidhewitt
Copy link
Member Author

Cool, I'll close this one. It looks like #862 works (all CI is green).

We can always resurrect this PR later if we still have trouble after #862.

@davidhewitt davidhewitt closed this Apr 9, 2020
@davidhewitt davidhewitt deleted the fix-datetime-test branch August 10, 2021 07:19
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.

Flaky datetime test
3 participants