Opened 15 years ago

Last modified 11 years ago

#1981 enhancement new

twisted.python.logfile silently refuses to rotate logs if it can't write to the directory

Reported by: Stephen Thorne Owned by: Andrii V. Mishkovskyi
Priority: lowest Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone Branch:
Author:

Description

13:57 < moshez> os.access is suck
13:57 < glyph> moshez: agreed!

os.access should be removed from twisted.python.logfile.

Attachments (1)

1981.patch (4.0 KB) - added by Andrii V. Mishkovskyi 11 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 15 years ago by Glyph

Priority: normallowest

comment:2 Changed 15 years ago by Glyph

Owner: changed from Glyph to Stephen Thorne

comment:3 Changed 14 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

Can you please clarify this ticket?

comment:4 Changed 14 years ago by Stephen Thorne

Summary: twisted.python.logfile cowardly, silently and wrongly refuses to rotate logs if it can't write to the directorytwisted.python.logfile silently refuses to rotate logs if it can't write to the directory

i've re-read the irc logs regarding this problem.

There are two tasks that would be nice to complete regarding this section of code, that is:

twisted.python.logfile.LogFile.rotate should attempt to rotate the logs instead of using os.access to decide if it can rotate the logs before attempting to do so. There are cases on systems where os.access will say we don't have write access when we do, (and vice versa).

We should log an error saying that we failed to rotate the logs, if we cannot rotate the logs.

comment:5 Changed 11 years ago by Andrii V. Mishkovskyi

Owner: changed from Stephen Thorne to Andrii V. Mishkovskyi

I started looking at this and can't seem to find any tests for LogFile in twisted/python/test Are there're any at all? As for the ticket itself -- can somebody clarify if not using os.access but instead actually trying to rotate sounds ok to everybody? And as for logging the inability to rotate log -- what format should be used to log that event?

comment:6 Changed 11 years ago by Andrii V. Mishkovskyi

So, the current code looks like this:

        if not (os.access(self.directory, os.W_OK) and os.access(self.path, os.W_OK)):
            return
        newpath = "%s.%s" % (self.path, self.suffix(self.lastDate))
        if os.path.exists(newpath):
            return
        self._file.close()
        os.rename(self.path, newpath)
        self._openFile()

It's definitely doesn't make sense to remove os.access(self.path, os.W_OK)), as otherwise after failed rotate we'll end up with no file to write to. Removing os.access(self.directory, os.W_OK) in favour of try ... except block does make sense though. As emitting warning and using try .. except for that sound like two distinct issues, i'm going to provide two patches.

comment:7 Changed 11 years ago by Andrii V. Mishkovskyi

Owner: Andrii V. Mishkovskyi deleted

Here's an attempt at a patch, with UserWarning being issued if rotate() method of both DailyLogFile and BaseLogFile can't actually rotate.

Changed 11 years ago by Andrii V. Mishkovskyi

Attachment: 1981.patch added

comment:8 Changed 11 years ago by Andrii V. Mishkovskyi

Keywords: review added

Well, duh, forgot to tag for review

comment:9 Changed 11 years ago by Drew Smathers

Owner: set to Drew Smathers
Status: newassigned

comment:10 Changed 11 years ago by Drew Smathers

Keywords: review removed
Owner: changed from Drew Smathers to Andrii V. Mishkovskyi
Status: assignednew

The only acceptance criterion I can gather from this ticket is "os.access should be removed from twisted.python.logfile." The current patch does not address the issue with os.access() which I gather to be it giving false negatives in some cases; try/except should be a more reliable way of telling we can't open a a file for writing.

Note: See TracTickets for help on using tickets.