Skip to content

bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules #9047

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
pitrou merged 7 commits into from
Feb 18, 2019
Merged

bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules #9047

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e47c5e1
change unpickling to use import rather than retrieving from sys.modules
tjb900 Sep 5, 2018
595bdbb
pep7 fixup
tjb900 Feb 7, 2019
f72802a
add unit test for bpo-34572
tjb900 Feb 7, 2019
fa5dbeb
add comment to explain why GetModule wasn't used
tjb900 Feb 8, 2019
64a7f95
rewrite test case to avoid sleeps
tjb900 Feb 8, 2019
202d4e2
move test to AbstractUnpickleTester
tjb900 Feb 8, 2019
a0425ef
better assertion
tjb900 Feb 13, 2019
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
Prev Previous commit
Next Next commit
add unit test for bpo-34572
  • Loading branch information
@tjb900
tjb900 committed Feb 7, 2019
commit f72802acfd2049285a7d2c1ce446f4991cdb8395
53 changes: 53 additions & 0 deletions Lib/test/test_threaded_pickle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import sys
import importlib
import os
import shutil
import threading
import unittest
from test.support import TESTFN, forget
import pickle

slow_import_module = """
import time

time.sleep(%(delay)s)

@pablogsal pablogsal Feb 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test relies on sleep to work. I do not feel comfortable with such tests as it may fail or not work as intended almost certainly in the slowest or more exotic buildbots that we have. Check out, for example, the discussion in #10413 .

As an additional note, it also needs the reap_threads decorator.

@pganssle pganssle Feb 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It will make the test a bit more complicated, but I think you can avoid sleep and make this more deterministic by creating a deliberate deadlock like this:

  1. Acquire Lock A in the test
  2. Attempt to acquire lock A in the partially-initialized module
  3. Spawn a thread that attempts to unpickle the object that involves an import
  4. Release lock A
  5. After the partially-initialized module has lock A, it adds the attribute that the pickled object uses.

I think there's still a race condition there that doesn't make it deterministic - namely that the thread which is unpickling the object needs to reach the "access the imported attribute" stage before lock A is released in order for it to be a good test, but I think if you fiddle around with the locks and threads a little bit you can get it to work.

@pitrou pitrou Feb 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll be more radical and say I don't think we need a test at all, if we can't make it simpler.

@pganssle pganssle Feb 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pitrou I think removing the test because it's not guaranteed to test the thing it's intended to test would be a mistake. This still successfully tests what it's intending to test with high probability, which means that a regression would be very unlikely to ever hit master. Unlike other tests which cause erroneous failures, this causes erroneous successes, so its flakiness is unlikely to cause any erroneously broken buildbots.

If left in as a "flaky" test, I'd probably view it similar to a hypothesis test, which is that it's a probabilistic test that never has false negatives - if the test succeeds you are only partially certain that you have no bugs, but if it fails you know you have a bug.

Elsewhere you mention adding a comment about why PyImport_GetModule was dropped to prevent regressions - if it's worth a comment (which is not guaranteed to work), it's probably worth at least trying to enforce this with a test. Possibly with a comment that says why the test isn't guaranteed to detect regressions.

@pitrou pitrou Feb 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, there are corner cases that we fix without writing tests for, because they are too tricky to reproduce. I think this one is a bit borderline: it's possible to reproduce, but nevertheless it needs a complex machinery.

@pganssle pganssle Feb 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I certainly agree that if it's likely to have a negative impact on the test suite for reasons other than that it's not guaranteed to fail in the case of a regression the test should probably be considered optional.

I do think it's worth trying to get a test for this in the test suite, even if just as an exercising in providing a good example of how to test a race condition.


class ToBePickled(object):
pass
"""


class ThreadedPickleTests(unittest.TestCase):
def test_pickle_thread_imports(self):
# https://bugs.python.org/issue34572
delay = 0.01
os.mkdir(TESTFN)
self.addCleanup(shutil.rmtree, TESTFN)
sys.path.insert(0, TESTFN)
self.addCleanup(sys.path.remove, TESTFN)
contents = slow_import_module % {'delay': delay}
with open(os.path.join(TESTFN, "slowimport.py"), "wb") as f:
f.write(contents.encode('utf-8'))
self.addCleanup(forget, "slowimport")

import slowimport
obj = slowimport.ToBePickled()
pickle_bytes = pickle.dumps(obj)
del obj

# Then try to unpickle two of these simultaneously
def do_unpickle():
results.append(pickle.loads(pickle_bytes))

for repeat_test in range(10):
results = []
forget("slowimport")
self.assertNotIn("slowimport", sys.modules)
importlib.invalidate_caches()

@pablogsal pablogsal Feb 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this affect any of the importlib tests?

@tjb900 tjb900 Feb 8, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any advice on how I can address this? FWIW I pretty much copied this test from test_threaded_import.py.

I haven't tested without that line, so it may not be necessary. I'll see if the test works (i.e. fails without the patch) reliably without it.

@pitrou pitrou Feb 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, I don't know why it would affect the importlib tests (I'm the one who added that function IIRC).

t1 = threading.Thread(target=do_unpickle)
t2 = threading.Thread(target=do_unpickle)
t1.start()
t2.start()
t1.join()
t2.join()
self.assertEqual(len(results), 2)