How to properly delete lockfile in a python abstract class?

Question

I have a python3 cron script that consists of a MyCron class and a MyIMAP class.

The MyCron class is an abstract class that makes sure only one instance of the script runs. It creates and destroys a lock file and throws a SingleInstanceExeption when cron tries to run the script when it's already running.

The MyIMAP class inherits the MyCron class as its base class. It checks email and returns unread emails. If anything goes wrong, I want my script to neatly close the connection and destroy the lockfile.

In both classes I am overriding __del__ method. In MyCron because I need to remove the lock, and in MyIMAP to close the connection.

I am experiencing weird results (objects no longer existing) when __del__ is invoked. Here's a sample of the code:

class MyCron(object):
    def __init__(self, jobname=os.path.basename(sys.argv[0]).split('.')[0]):
        self.logger = logging.getLogger(__name__)
        self.initialized = False
        lockfilename  = "mycron"
        lockfilename += "-%s.lock" % jobname if jobname else ".lock"
        self.lockfile = os.path.normpath(tempfile.gettempdir() + '/' + lockfilename)
        self.logger.debug("MyCron lockfile: " + self.lockfile)
        self.fp = open(self.lockfile, 'w')
        self.fp.flush()
        try:
            fcntl.lockf(self.fp, fcntl.LOCK_EX | fcntl.LOCK_NB)
            self.initialized = True
        except IOError:
            self.logger.warning("Another %s MyCron is already running, quitting." % jobname)
            raise SingleInstanceException()
            self.initialized = False

    def __del__(self):
        if not self.initialized:
            return
        try:
            fcntl.lockf(self.fp, fcntl.LOCK_UN)
            # os.close(self.fp)
            if os.path.isfile(self.lockfile):
                os.unlink(self.lockfile)
        except Exception as e:
            if self.logger:
                self.logger.warning(e)
            else:
                print("Unloggable error: %s" % e)
            sys.exit(-1)

class MyIMAP(MyCron):
    def __init__(self, server, username, password, port=993, timeout=60):
        super(MyIMAP, self).__init__()
        self.server     = server
        self.username   = username
        self.password   = password
        self.port       = port
        self.connection = None
        if self.initialized:
            socket.setdefaulttimeout(timeout)
            self.connect()
            self.login()

    def __del__(self):
        super(MyIMAP, self).__del__()
        if self.initialized and self.connection:
            self.connection.logout()
            self.logger.info("Close connection to %s:%d" % (self.server, self.port))

    ...

I understand this is related to the unpredictable nature of the __del__ method, and I should probably be implementing this a different way. What's the best practice here for python 3?

[`with`](https://docs.python.org/3/reference/compound_stmts.html#with), basically always [`with`](https://docs.python.org/3/reference/compound_stmts.html#with). However that will require you to move away from the "do everything in `__init__()` approach" that you currently have. — dhke, Sep 11 '17 at 17:45
I understand this is likely the solution, but how does that look in my script? When I call MyIMAP, it returns an active connection (mailbox) object. It actually does a bit more than just return unread emails. So the top of my script would have a `with` statement and the rest of the script would be indented under that statement? Kind of ugly, no? — pixelrebel, Sep 11 '17 at 18:01
`__del__` is pretty much exactly not for that kind of use. The inheritance also seems a little pointless and just makes it harder to reason about the code. — pvg, Sep 11 '17 at 18:01
The inheritance is because I have many different kinds of related cron scripts running. This example has to do with IMAP checking. — pixelrebel, Sep 11 '17 at 18:03
Right, but that still doesn't really require inheritance. But that's a separate issue. The main thing is, you should not be using `__del__` for resource management, as the other commenter pointed out, that's what `with` is for. — pvg, Sep 11 '17 at 18:07
So, if I want to return an active connection object, the whole logic needs to be indented under a `with` statement? That's like 100 lines of code under a `with` statement. I've never seen that before in practice. I must be going down the wrong path here, but I'm not sure how to better structure this. — pixelrebel, Sep 11 '17 at 18:20
That sounds like a different set of questions and maybe you can review your design and ask about specifics as you come up with them. As it stands, your question boils down to 'how do I strongly couple my object lifecycles to system resources'. The answer to that is 'you don't, that's a bad idea, as you are finding out'. Look into context handlers/with, look up `atexit` shutdown handlers,don't tie your design to an existence of a single instance which does 99% of its job in its constructor (you don't need an instance of anything to know you need to abort if lockfile, for example) etc, etc — pvg, Sep 11 '17 at 18:24
"*100 lines of code under `with` statement*" sounds more like "*I've got a bad case of spaghetti code*". What's wrong with `with construct() as obj: do_stuff(obj)`? — dhke, Sep 11 '17 at 18:43
Okay, thanks @pvg for the guidance. So, would you suggest I throw out the `MyCron` abstract class and handle the lockfile in main portion of the script? I suppose if I approach that way, I have to be more careful to catch any exceptions or that lockfile will not get cleaned up. That was my primary concern and reasoning behind the abstract `MyCron` class. — pixelrebel, Sep 11 '17 at 18:45

How to properly delete lockfile in a python abstract class?

0 Answers0