@pganssle - I'm trying to figure out how my pytest failed on the AppVeyor CI. It failed on a line that asserts that the weak references are cleared immediately following a couple del statements and a gc.collect() statement.

Any ideas how this could happen? Is it possible that the gettz tests are running twice asynchronously? I noticed that if I comment out the test, the count of passing test cases jumps from 1700 to 1698. Maybe I have race condition introduced somewhere -- if two test runs happen to occur at the same time, one could be late to run a del on a strong reference and the data doesn't get cleaned up.

Hard to tell if this is just holding a strong reference somewhere we don't realize or if the assumptions about one of the inputs is wrong. Seems to only happen on Windows on old versions of Python, though.

Sorry for the flurry of seemingly random commits. I can't seem to replicate AppVeyor's failure on my local machine even after matching the Python version, so I'm trying to troubleshoot with small changes.

Just reverted all the troubleshooting commits, but I'm kind of at a loss here for how this is failing on the AppVeyor CI.

The stdout from the 46bdab7 commit is really what's really tripping me up:
https://ci.appveyor.com/project/dateutil/dateutil/build/2.3.1234/job/4ru7kdg7wipal15w

In this commit, I print the keys inside the WeakValueDictionary before and after deleting the strong ATK1 and ATK2 references, expecting the WeakValueDictionary to clear itself, but it does not. I also used pytest.raises(NameError) to assert that ATK1 and ATK2 are indeed deleted, and they are. There shouldn't be any remaining strong references to the value in the dictionary, but somehow it is persisting. Running the equivalent code locally off of CI yields the expected behavior.

I'll keep thinking about how to get around this. One idea I had was to have some kind of _cache_hit attribute on the factory that keeps a count of every time it is returning a value from its WeakValueDictionary, and not incrementing when it's creating a brand new instance. We can assert that this counter does/does not change depending on the test.

I'm guessing that this is a real issue with Windows and not a problem with the tests. I'll grab this branch and try debugging it on a Windows VM.

Ah, I figured this out. It's because ZoneInfoFile is read once, so it actually contains strong references to each of the tzfile objects it created. For now we'll mark it as xfail on Windows, and either it'll be solved by moving zoneinfo over to using a weak reference cache or we do some kind of thing where we install zoneinfo files somewhere and modify the Windows path as part of the tests, or just change that particular one to a skipif.

Cleaned up the commit history. Will merge when tests pass.

Thanks so much @cs-cordero!

Happy to help. Thanks for taking it home with that clutch Windows ZoneInfoFile issue.

Hm.. The Python 3.5 test failed but succeeded on reboot. This "id won't get reused" gambit may be less stable than I hoped.

I just had a new idea for a more robust test, though. Create a new weak reference to the object before deleting it and make sure its referrent is gone after the gc step.

@cs-cordero One last thing, can you add yourself to AUTHORS.md - names are in alphabetical order. You are welcome to put your real e-mail and encouraged to put github name. Make sure to put a D after your name line, which indicates your contributions are under the dual license.

Will do re: authors.md once I get access to a computer and little later in the day. Thanks!

@cs-cordero No this test assumes that the weakref module works as expected and tests that we didn't screw up and accidentally start holding a strong reference to the tzinfo objects in the cache.

If you do:

# Checkout base of the branch
git checkout e6644a4
# Checkout the commit where the tests were added
git checkout 75ae199 -- dateutil/test/test_tz.py
pytest -k test_tz.py

You'll see that these three tests fail, which means the problem - that the cache itself holds a strong reference to the cached time zones - is solved by this patch.

Ah yes that makes perfect sense! Thank you for explaining.

@cs-cordero You'll probably need to do a force pull when you edit this branch again, because I've rebased your branch and rewritten the history. If you don't know how to do that it's git pull --force

🎉