Skip to content

bpo-38018: increase code coverage for multiprocessing.shared_memory #15662

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

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

vinay0410
Copy link
Contributor

@vinay0410 vinay0410 commented Sep 3, 2019

Copy link
Member

@applio applio left a comment

Choose a reason for hiding this comment

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

The first two checks look like good additions.
The use of mock to control the name of the shared memory segments does not look right to me -- have you verified that this test passes?

@vinay0410
Copy link
Contributor Author

@applio , I have verified that the test passes. Also, I have verified that it covers these lines.
I wanted to add a test case which verifies the fact that nothing breaks, if _make_filename generates name of already existing segment.
I only could ensure that using mock.

Copy link
Member

@applio applio left a comment

Choose a reason for hiding this comment

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

The test involving mock appears use two distinct names for the two SharedMemory(create=True, size=1) invocations. Crudely adding a print() to show shm1.name and shm2.name seems to confirm this. Hence the two tests that pass are against names[0] and later names[1].

Perhaps I misunderstood but I thought the intent was to verify that _make_filename creating duplicate names does not trigger a problem in and of itself.

@vinay0410
Copy link
Contributor Author

vinay0410 commented Sep 9, 2019

@applio, The intent of the test is to verify if a problem is caused when _make_filename returns as existing name.
So, the first call shm1 = shared_memory.SharedMemory(create=True, size=1) will create a shared memory with name test01_fn, since such a name(shared memory) doesn't exists yet.

Then I reset the side effect function of the mock object by mock_make_filename.side_effect = names

So, _make_filename will now again return test01_fn on first call. And, test01_fn is now a name of existing shared memory segment, which was created by the first call.
This will result in a FileExistsError here

And then the loop will continue, calling _make_filename again, which will then return test02_fn, and since no shared memory with such name exists, it will be created.

So, therefore it verifies that existing segment names returned by _make_filename will be skipped, and it will be called again.

@applio
Copy link
Member

applio commented Sep 9, 2019

Ahhh, thank you for the clarification. Got it.

@applio applio merged commit d14e39c into python:master Sep 9, 2019
@miss-islington
Copy link
Contributor

Thanks @vinay0410 for the PR, and @applio for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @vinay0410 and @applio, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker d14e39c8d9a9b525c7dcd83b2a260e2707fa85c1 3.8

@pablogsal
Copy link
Member

This PR introduced a regression in FreeBSD buildbots:

https://buildbot.python.org/all/#/builders/168/builds/1417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants