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

Made the log method a weakreference #1671

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

spatankar-dmpna
Copy link

This is a fix for issue 1670. The issue outlines a bug where garbage collector cannot destroy the LoggingConnection object since it has a bound method called log on it.

lib/extras.py Outdated
"""
Public interface of the log method defined in initialize
"""
return self._log()(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

You should check if self._log() returns None. In that case I think you can just plonk the message: it likely means that the program is being terminated.

Copy link
Author

@spatankar-dmpna spatankar-dmpna Feb 6, 2024

Choose a reason for hiding this comment

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

So you mean something like this?
`

if self._log():
     return self._log()(*args, **kwargs)

`

Copy link
Member

@dvarrazzo dvarrazzo Feb 6, 2024

Choose a reason for hiding this comment

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

No, more:

log = self._log()
if log:
    log(*args, **kwargs)

otherwise there is a race condition.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not a 100% sure of this myself but wouldn't this replicate the same bound method issue again?

It is possible that the WeaktMethod does decrease the reference count even after we do this though.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a 100% sure of this myself but wouldn't this replicate the same bound method issue again?

No, because this would be into the log() function. log, here, is a local variable.

Copy link
Author

Choose a reason for hiding this comment

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

I also wanted to ask if this issue might be happening in other parts. I could make the same changes there too.

@dvarrazzo
Copy link
Member

I played a bit more with the issue. I didn't realise that the pattern of copying a bound method to the state creates a loop. It might be somewhere else too, sure; I have in mind psycopg 3 date/time adapters; I don't know about psycopg2.

However, looking at it, I don' think I would use weakref at all. ISTM that the same can be achieved by taking a class method reference in the state, instead of the bound method. Something like, untested:

        if _logging and isinstance(
                logobj, (_logging.Logger, _logging.LoggerAdapter)):
            self._log_method = type(self)._logtologger
        else:
            self._log_method = type(self)._logtofile

    def log(self, msg, curs):
        """
        Public interface of the log method defined in initialize
        """
        self._log_method(self, msg, curs)

@dvarrazzo
Copy link
Member

...or, even cleaner, make _logtologger, _logtofile two staticmethods (taking a self as first parameter anyway) and avoid taking them from type(self) in the constructor.

@dvarrazzo
Copy link
Member

See psycopg/psycopg@36b0a55 for an example of the same problem addressed in psycopg 3.

@dvarrazzo
Copy link
Member

As for whether there are similar cases in psycopg2, using ag 'self\..* = self\.' I could only see the pattern used in the hstore:

psycopg2/lib/extras.py

Lines 797 to 798 in 0087054

if conn.info.server_version < 90000:
self.getquoted = self._getquoted_8

It's worth fixing it for consistency, however that case only happens with hstore before Postgres 9.0 it seems, so only with long unsupported Postgres versions.

@spatankar-dmpna
Copy link
Author

Why is taking self as a parameter for static methods better than using a weak reference for the bound method? Everything I've been looking into tells me not to do that. My PyLint checker throws up a warning when I attempt to do that.

@dvarrazzo
Copy link
Member

dvarrazzo commented Feb 16, 2024 via email

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.

2 participants