Home Original page

bpo-34722: Consistent serialization of sets in bytecode by peterebden · Pull Request #9472 · python/cpython

Conversation

@peterebden

This ensures that sets / frozensets marshal in a consistent order by sorting the serialised items before writing them. That will obviously make serialisation of such types somewhat slower.

Added a test for the case in question; it needs to use compileall as a subprocess in order to test with different hash seeds.

https://bugs.python.org/issue34722

serhiy-storchaka

Choose a reason for hiding this comment

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

This will lost the identity of objects. For example, in {(o, 1), (o, 2)} you will get different objects after marshalling/unmarshalling.

Choose a reason for hiding this comment

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

I'm not sure I can get the same objects back at present?

    def test_object_identity(self):
        o = 'test'
        obj = {(o, 1), (o, 2)}
        data = marshal.dumps(obj)
        v = marshal.loads(data)
        ids_before = {id(x) for x in obj}
        ids_after = {id(x) for x in marshal.loads(data)}
        self.assertEqual(ids_before, ids_after)

AssertionError: Items in the first set but not the second:
139864671036808
139864645838728

Maybe I'm misunderstanding what you mean, or that's not a good test case?

Choose a reason for hiding this comment

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

Additional tests were added in #13736. Rebase your PR and make the tests be success.

ZackerySpytz

w_object(value, p);
Py_DECREF(value);
for (i = 0; i < n; i++) {
value = PyList_GetItem(l, i);

Choose a reason for hiding this comment

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

I think the PyList_GetItem() and PyList_SetItem() calls added in this patch should be replaced with the PyList_GET_ITEM() and PyList_SET_ITEM() macros.

Also, PyMarshal_WriteObjectToString() should be checked for failure.

@tiran tiran removed their request for review

April 17, 2021 21:03

@methane

Close this because #27926 was merged.

Labels