Opened 3 years ago

Last modified 2 months ago

#5723 defect new

LogFile.rotate is overly careful in its LBYL code

Reported by: exarkun Owned by: mriehl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/logfile-rotate-5723
(diff, github, buildbot, log)
Author: adiroiban Launchpad Bug:

Description

       if not (os.access(self.directory, os.W_OK) and os.access(self.path, os.W_OK)):
            return

Write access to a file is not necessary to rename that file. The second part of this check is superfluous and sometimes counterproductive (as the process may have write permission to the directory but not the file). It should be removed.

Additionally, look-before-you-leap is a bad way to write software. Instead, the code should probably just try rotating the file. If it fails with a permission error, that can be silenced.

This may have some relationship with #4707 as well.

Attachments (1)

patch-for-5723.patch (7.9 KB) - added by mriehl 9 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by exarkun

#6073 was a duplicate of this.

Changed 9 months ago by mriehl

comment:2 Changed 9 months ago by mriehl

  • Keywords review added

comment:3 Changed 9 months ago by mriehl

A few notes:

  • IMHO there is a lot of stylistic work left to do on logfile.py (unit test naming conventions not consistent for example) but I wanted to keep this patch as small as possible
  • I fixed some minor docstrings and formatting issues (boyscout)
  • The tests are not unit tests. They perform file I/O - I'd normally use mock for this, but could not find any usage in the codebase. Since all the existing tests also do file I/O, I went with this approach too

comment:4 follow-up: Changed 9 months ago by exarkun

Thanks!

IMHO there is a lot of stylistic work left to do on logfile.py (unit test naming conventions not consistent for example) but I wanted to keep this patch as small as possible

Thank you. This is the preferred approach. :)

The tests are not unit tests. They perform file I/O - I'd normally use mock for this, but could not find any usage in the codebase. Since all the existing tests also do file I/O, I went with this approach too

The local filesystem is almost always available, almost always reliable, it is fast, it is rather deterministic. The benefits of faking it are quite minor whereas the costs of doing so (well) are somewhat high. This isn't to say no one should ever do this but done wrong the solution is worse than the problem and done right this isn't a small undertaking.

comment:5 in reply to: ↑ 4 Changed 9 months ago by mriehl

Replying to exarkun:

The local filesystem is almost always available, almost always reliable, it is fast, it is rather deterministic. The benefits of faking it are quite minor whereas the costs of doing so (well) are somewhat high. This isn't to say no one should ever do this but done wrong the solution is worse than the problem and done right this isn't a small undertaking.

I understand. Another argument would be cross-platform since when using mock you're only micking behaviour and would probably miss some quirks, e.G. on windows.

comment:6 Changed 6 months ago by habnabit

  • Owner set to habnabit

comment:7 Changed 2 months ago by adiroiban

  • Owner changed from habnabit to adiroiban

I will create a branch and send this to buildbot

comment:8 Changed 2 months ago by adiroiban

  • Author set to adiroiban
  • Branch set to branches/logfile-rotate-5723

(In [43782]) Branching to logfile-rotate-5723.

comment:9 Changed 2 months ago by adiroiban

  • Keywords review removed
  • Owner changed from adiroiban to mriehl

Hi mriehl,

Thanks for the patch and sorry for the long delay.

All new code, including test methods require docstrings.
Test docstring should describe what is the purpose of the test.

Please see this report https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/2512/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

A patch also needs a news file. See http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles


Don't know what to say about inline comments, and inline comments indentation, especially multi lines comments. Looks like twistedchecker is fine with this.

By reading the other part of the twisted code I saw that block comments are preferred.

so instead of

+        log.rotate()  # renaming the old logfile will raise an OSError,
+                      # which should be caught

you will have

+        # Renaming the old logfile will raise an OSError,
+        # which should be caught.
+        log.rotate()  

Comments should be proper sentences, phrases with full stop.


Regarding The following operations might fail silently I would avoid maintaining a comprehensive list of actions, as latest the docstring might get out of sync.

I think that this should be enough

In case the rotation of the current logfile fails, the current logfile
will be used for further logging.

but feel free to keep this if you prefer.


This is an old code.. but new test methods should be called test_*.


Other than the above comments, it looks good.

Please address the issues and send a new patch. Thanks!

Note: See TracTickets for help on using tickets.