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

RotatingFileHandler rollover doesn't work on QNX shmem filesystems #60653

Closed
phmc mannequin opened this issue Nov 10, 2012 · 12 comments
Closed

RotatingFileHandler rollover doesn't work on QNX shmem filesystems #60653

phmc mannequin opened this issue Nov 10, 2012 · 12 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@phmc
Copy link
Mannequin

phmc mannequin commented Nov 10, 2012

BPO 16449
Nosy @vsajip, @pitrou, @bitdancer, @serhiy-storchaka, @phmc
Files
  • rfh_rename_fix.patch: Proposed patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2012-11-11.11:45:46.821>
    created_at = <Date 2012-11-10.14:36:10.266>
    labels = ['type-feature', 'library']
    title = "RotatingFileHandler rollover doesn't work on QNX shmem filesystems"
    updated_at = <Date 2012-11-11.16:41:24.206>
    user = 'https://github.com/phmc'

    bugs.python.org fields:

    activity = <Date 2012-11-11.16:41:24.206>
    actor = 'vinay.sajip'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-11-11.11:45:46.821>
    closer = 'vinay.sajip'
    components = ['Library (Lib)']
    creation = <Date 2012-11-10.14:36:10.266>
    creator = 'pconnell'
    dependencies = []
    files = ['27941']
    hgrepos = []
    issue_num = 16449
    keywords = ['patch']
    message_count = 12.0
    messages = ['175277', '175282', '175284', '175350', '175352', '175359', '175360', '175362', '175365', '175366', '175368', '175371']
    nosy_count = 5.0
    nosy_names = ['vinay.sajip', 'pitrou', 'r.david.murray', 'serhiy.storchaka', 'pconnell']
    pr_nums = []
    priority = 'normal'
    resolution = 'wont fix'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue16449'
    versions = ['Python 2.7', 'Python 3.3']

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Nov 10, 2012

    logging.handlers.RotatingFileHandler.doRollover fails on QNX /dev/shmem filesystems (seen on a 6.4.0-based system).

    QNX RAM filesystems don't support rename() (see http://www.qnx.com/developers/docs/6.4.0/neutrino/sys_arch/fsys.html#DEVSHMEM - it's a 'big filesystem' feature).

    So for example, rename("/dev/shmem/foo", "/dev/shmem/bar") fails with EXDEV.

    This is easily fixed by using shutils.move rather than os.rename where appropriate, falling back to copying if a rename() fails. It's not sufficient to set the rotator attribute, since there are other os.rename calls in in doRollover.

    @phmc phmc mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Nov 10, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Nov 10, 2012

    I'm not sure why you would setup logging on a RAM filesystem (I assume we're talking about normal volatile RAM)?
    But besides, the big point of using rename() is that it's fast, atomic and won't disrupt existing file handlers pointing to that file. If RotatingFileHandler may resort to copying by mistake, it may cause other issues down the line.

    @bitdancer
    Copy link
    Member

    I agree with Antoine. It seems to me that it is very important to the semantics of rollover that the rename be atomic, even if we ignore the issue of existing other readers. If it were not atomic, you might end up with lost log messages. So I don't think there is anything to do here: you just can't use rollover on a filesystem that doesn't support atomic rename.

    I'll leave it Vinay to close the issue if he agrees with us.

    @vsajip
    Copy link
    Member

    vsajip commented Nov 11, 2012

    Thanks for the patch, but I'm closing this as 'wontfix', as per the points made by Antoine and David. If you need logging from an embedded system, please consider using one of the socket-based logging handlers, if that's feasible in the specific situation.

    @vsajip vsajip closed this as completed Nov 11, 2012
    @serhiy-storchaka
    Copy link
    Member

    What about the "rotator" attribute.

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Nov 11, 2012

    I'm not convinced that it matters whether the rename or move is atomic. Can anyone come up with a quick concrete example?

    I see two scenarios:

    1. The process crashes during a copy in shutils.move(). In this case, some logs will be duplicated across the files involved in the copy.

    2. Other threads emit logs during the rollover. Given that the IO lock is acquired in Handler.handle() before calling emit(), this is fine.

    While the first case isn't ideal, I don't think there can be any loss of logs.

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Nov 11, 2012

    Serhiy, there are also calls to os.rename in RotatingFileHandler.doRollover

    @vsajip
    Copy link
    Member

    vsajip commented Nov 11, 2012

    The current implementation was written with an expectation of working rename functionality in the stdlib. As such, while this issue might be categorised as being of type "enhancement", I don't see how you can categorise it as being of type "behaviour".

    What's the problem with subclassing the relevant handlers to implement your own doRollover() method? You only need to do that once for all the projects in your QNX environment. Given that it's not a mainstream environment, that's not IMO an unreasonable suggestion.

    If you publish such a handler, I'll mention it on

    http://plumberjack.blogspot.co.uk/p/handlers-for-logging.html

    which has a whole bunch of handlers written by people for specific requirements which don't warrant adding functionality directly in the stdlib.

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Nov 11, 2012

    I've updated the type to enhancement (it seems like a grey area to me - it's a behavioural fix for a niche use case).

    I suggested a patch rather than simply subclassing RotatingFileHandler since:

    • The subclass would just have a copy of RotatingFileHandler's doRollover method with a one-line change.
    • The proposed fix is functionally equivalent to the current code for all currently working uses.

    Thanks for the offer of having an extension linked from your blog. I don't think it's appropriate for this case (since it's just a slightly modified copy of the stdlib code - I'll probably just keep the patch along with a few other compatibility hacks we have) although I may have something for you in future (we do have a subclass that compresses old logs).

    @phmc phmc mannequin added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Nov 11, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Nov 11, 2012

    I've updated the type to enhancement (it seems like a grey area to me

    • it's a behavioural fix for a niche use case).

    I suggested a patch rather than simply subclassing RotatingFileHandler
    since:

    • The subclass would just have a copy of RotatingFileHandler's
      doRollover method with a one-line change.
    • The proposed fix is functionally equivalent to the current code
      for all currently working uses.

    You may just as well monkeypatch os.rename() to fallback on
    shutil.move() if the filenames are on a /dev/shm filesystem (or you
    could bug QNX to fix their broken filesystem...).

    From a code quality and readability standpoint, os.rename() conveys the
    intended semantics clearly, while shutil.move() doesn't, so switching to
    shutil.move() in the stdlib would be a regression. Also, doing this in
    logging would open the door to doing the same thing in other modules.
    Even a critical piece of infrastructure such as importlib relies on
    os.rename() working properly.

    @serhiy-storchaka
    Copy link
    Member

    Doesn't the "rotator" attribute break atomicity? A careful rotator should first rename the source to the temporary file, process the data and save it to other temporary file, and then rename the result to the destination and remove the first temporary file.

    @vsajip
    Copy link
    Member

    vsajip commented Nov 11, 2012

    Doesn't the "rotator" attribute break atomicity?

    Which rotator do you mean? The default rotator is None, which leads to os.rename being called. If you're referring to the example in the documentation (cookbook) - it was intended purely as an example, and the paragraph under the snippet does say it's only intended for illustrative purposes.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants