Skip to content
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

Remove potentially-buggy positive_id #384

Merged
merged 3 commits into from
May 26, 2023
Merged

Remove potentially-buggy positive_id #384

merged 3 commits into from
May 26, 2023

Conversation

navytux
Copy link
Contributor

@navytux navytux commented May 26, 2023

positive_id was added in 2004 in f7b96ae (ZODB.utils grows a new function positive_id(), which returns the id() of an object as a non-negative integer ...) to workaround change in behaviour Python2.3 -> 2.4 for '%x' % <negative-number>. And nowdays positive_id is used only in one place in Connection.repr to mimic approximately what Python already gives for a default repr out of the box.

On https://github.com/zopefoundation/ZODB/actions/runs/5067242751/jobs/9098062772 we recently hit

ZODB.POSException.InvalidObjectReference: <unprintable InvalidObjectReference object>

for the following code:

if self._jar.get_connection(database_name) is not obj._p_jar:
    raise InvalidObjectReference(
        "Attempt to store a reference to an object from "
        "a separate connection to the same database or "
        "multidatabase", self._jar, obj,
    )

( https://github.com/zopefoundation/ZODB/blob/5.8.0-24-g8a7e3162d/src/ZODB/serialize.py#L366-L371 )

for which the "unprintable ..." part is produced by Lib/traceback.py who tries to do str(ex) and returns unprintable ... if that str failed:

https://github.com/python/cpython/blob/v3.8.16-18-g9f89c471fb1/Lib/traceback.py#L153-L157

Since one of the reasons for that failure might have been a bit non-trivial code and assert in positive_id, let's remove it and rely on builtin python behaviour for Connection repr.

repr for Connection:

before this patch:

<Connection at 0x7fef09989d10>

after this patch:

<ZODB.Connection.Connection object at 0x7fef09989d10>

positive_id was added in 2004 in f7b96ae (ZODB.utils grows a new
function positive_id(), which returns the id() of an object as a
non-negative integer ...) to workaround change in behaviour Python2.3 ->
2.4 for `'%x' % <negative-number>`. And nowdays `positive_id` is used
only in one place in Connection.__repr__ to mimic approximately what
Python already gives for a default __repr__ out of the box.

On https://github.com/zopefoundation/ZODB/actions/runs/5067242751/jobs/9098062772
we recently hit

    ZODB.POSException.InvalidObjectReference: <unprintable InvalidObjectReference object>

for the following code:

    if self._jar.get_connection(database_name) is not obj._p_jar:
        raise InvalidObjectReference(
            "Attempt to store a reference to an object from "
            "a separate connection to the same database or "
            "multidatabase", self._jar, obj,
        )

    ( https://github.com/zopefoundation/ZODB/blob/5.8.0-24-g8a7e3162d/src/ZODB/serialize.py#L366-L371 )

for which the "unprintable ..." part is produced by `Lib/traceback.py`
who tries to do `str(ex)` and returns `unprintable ...` if that str
failed:

    https://github.com/python/cpython/blob/v3.8.16-18-g9f89c471fb1/Lib/traceback.py#L153-L157

Since one of the reasons for that failure might have been a bit
non-trivial code and assert in positive_id, let's remove it and rely on
builtin python behaviour for Connection repr.

repr for Connection:

before this patch:

    <Connection at 0x7fef09989d10>

after this patch:

    <ZODB.Connection.Connection object at 0x7fef09989d10>
Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

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

Any change that removes homegrown replacements for built-in functionality is a win.

@navytux
Copy link
Contributor Author

navytux commented May 26, 2023

Thanks, Jens.

@navytux navytux requested a review from d-maurer May 26, 2023 13:18
@navytux
Copy link
Contributor Author

navytux commented May 26, 2023

Dieter, are you ok with this patch?

@navytux
Copy link
Contributor Author

navytux commented May 26, 2023

Thanks, Dieter.

@navytux navytux merged commit 12d1a05 into master May 26, 2023
28 checks passed
@navytux navytux deleted the y/kill-positive_id branch May 26, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants