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
bpo-39584: raise ValueError when creating shared memory of size greater than 1TB #21877
Conversation
7ca8853
to
7468c1c
Compare
Hi @ronaldoussoren , I have made changes to python |
reopened PR to re-trigger CI |
@@ -0,0 +1,2 @@ | |||
Prevent creating :class:`shared_memory.SharedMemory` objects with | |||
:code:`size` greater than 1TB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a valid use of the code
role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you have a look at this: #21556 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Okay. But double backquotes are usually used in cases like this (as specified by the developer's guide https://devguide.python.org/documenting/#inline-markup).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess both of them work. See this https://devguide.python.org/documenting/#module-specific-markup .
I was going for this because it is tried and tested.
Although, if you still want me to change to double back ticks, I will make changes.
@@ -3880,6 +3880,10 @@ class OptionalAttachSharedMemory(shared_memory.SharedMemory): | |||
|
|||
sms.close() | |||
|
|||
# Test creating a shared memory segment with size greater than shared_memory._MAX_SHM_SIZE_LIMIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need for this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this comment because similar tests around this test use such comments.
Lib/test/_test_multiprocessing.py
Outdated
@@ -3880,6 +3880,10 @@ class OptionalAttachSharedMemory(shared_memory.SharedMemory): | |||
|
|||
sms.close() | |||
|
|||
# Test creating a shared memory segment with size greater than shared_memory._MAX_SHM_SIZE_LIMIT | |||
with self.assertRaises(ValueError): | |||
sms_invalid = shared_memory.SharedMemory(create=True, size=shared_memory._MAX_SHM_SIZE_LIMIT + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for a variable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree, I will remove that.
But, if you see there are similar tests around this test assigning an unnecessary variable, should I remove those too ? Or should raise another pull request removing those.
6eaa6da
to
691f27c
Compare
691f27c
to
84bb92d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes the limit for all systems, not just on macOS. I'd prefer to have the limit only on macOS, the behaviour of other unix-y systems (and in particular Linux) is different and likely don't need this artificial limit.
if not isinstance(item, (str, bytes)) | ||
else self._types_mapping[type(item)] % ( | ||
self._alignment * (len(item) // self._alignment + 1), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mix actual changes with formatting changes.
@@ -422,7 +431,8 @@ def _set_packing_format_and_transform(self, position, fmt_as_str, value): | |||
def __getitem__(self, position): | |||
position = position if position >= 0 else position + self._list_len | |||
try: | |||
offset = self._offset_data_start + self._allocated_offsets[position] | |||
offset = self._offset_data_start + \ | |||
self._allocated_offsets[position] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise
@@ -449,7 +459,8 @@ def __setitem__(self, position, value): | |||
new_format = self._types_mapping[type(value)] | |||
encoded_value = value | |||
else: | |||
allocated_length = self._allocated_offsets[position + 1] - item_offset | |||
allocated_length = self._allocated_offsets[position + | |||
1] - item_offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Also: I don't feel qualified to approve changes to multiprocessing. This is a complex library that I don't use myself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read my most recent comment on bpo. 1 TB is arbitrary and far too low.
I'm closing this PR since the solution here has been rejected on the issue. |
https://bugs.python.org/issue39584