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

dumbdbm should not commit if in read mode #73033

Closed
joanthanng mannequin opened this issue Dec 1, 2016 · 13 comments
Closed

dumbdbm should not commit if in read mode #73033

joanthanng mannequin opened this issue Dec 1, 2016 · 13 comments
Assignees
Labels
3.7 stdlib type-feature

Comments

@joanthanng
Copy link
Mannequin

@joanthanng joanthanng mannequin commented Dec 1, 2016

BPO 28847
Nosy @vadmium, @serhiy-storchaka, @joanthanng
PRs
  • #552
  • Files
  • dbm_dumb_readonly.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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2016-12-07.09:12:22.519>
    created_at = <Date 2016-12-01.03:50:06.052>
    labels = ['3.7', 'type-feature', 'library']
    title = 'dumbdbm should not commit if in read mode'
    updated_at = <Date 2017-03-31.16:36:22.839>
    user = 'https://github.com/joanthanng'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:22.839>
    actor = 'dstufft'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-12-07.09:12:22.519>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2016-12-01.03:50:06.052>
    creator = 'Jonathan Ng'
    dependencies = []
    files = ['45722']
    hgrepos = []
    issue_num = 28847
    keywords = ['patch']
    message_count = 13.0
    messages = ['282133', '282158', '282166', '282167', '282170', '282218', '282219', '282264', '282265', '282266', '282269', '282271', '282602']
    nosy_count = 4.0
    nosy_names = ['python-dev', 'martin.panter', 'serhiy.storchaka', 'Jonathan Ng']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue28847'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @joanthanng joanthanng mannequin added the type-bug label Dec 1, 2016
    @joanthanng
    Copy link
    Mannequin Author

    @joanthanng joanthanng mannequin commented Dec 1, 2016

    Or at the very least, if there is an OSError in _update, an error should be raised instead of ignoring this error.

    In the current state of the code, if there was an OSError while reading the dirfile, the dirfile would be overwritten with a blank file when it is closed.

    @joanthanng joanthanng mannequin changed the title dumbdbm should not commit if dumbdbm should not commit if in read mode Dec 1, 2016
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 1, 2016

    Do you have concrete example when not ignoring an OSError in _update causes an issue? This is needed for writing tests.

    @serhiy-storchaka serhiy-storchaka added the stdlib label Dec 1, 2016
    @joanthanng
    Copy link
    Mannequin Author

    @joanthanng joanthanng mannequin commented Dec 1, 2016

    I'm not sure how to create an OSError.

    But perhaps something like this:

    '''
    from dbm import dumb
    import os

    db = dumb.open('temp', flag='n')
    db['foo'] = 'bar'
    db.close()
    
    db = dumb.open('temp', flag='r')
    print(len(db.keys()))
    db.close
    
    os.rename('temp.dir', 'temp_.dir') # simulates OSError
    db = dumb.open('temp', flag='r')
    os.rename('temp_.dir', 'temp.dir')
    db.close()
    
    db = dumb.open('temp', flag='r')
    assert len(db.keys()) > 0
    '''

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 1, 2016

    This example is too artificial.

    But there is a real issue: opening read-only files in read mode. Currently this causes a PermissionError on closing.

    For backward compatibility flags 'r' and 'w' are ignored. I.e. opening with 'r' and 'w' creates a file if it is not existing, and opening with 'r' allows modifying the database. Since 3.6 this emits deprecation warnings (bpo-21708). In future versions this will be an error.

    Proposed patch makes two changes:

    1. The index file no longer written if the database was not modified. This increases performance and adds a support of read-only files.

    2. A deprecation warning is raised when the index file is absent in 'r' and 'w' modes. In future versions this will be an error.

    May be the first change can be backported.

    @serhiy-storchaka serhiy-storchaka added 3.7 type-feature and removed type-bug labels Dec 1, 2016
    @joanthanng
    Copy link
    Mannequin Author

    @joanthanng joanthanng mannequin commented Dec 1, 2016

    #1 makes sense to be backported.

    On Thu, Dec 1, 2016 at 8:41 PM, Serhiy Storchaka <report@bugs.python.org>
    wrote:

    Serhiy Storchaka added the comment:

    This example is too artificial.

    But there is a real issue: opening read-only files in read mode. Currently
    this causes a PermissionError on closing.

    For backward compatibility flags 'r' and 'w' are ignored. I.e. opening
    with 'r' and 'w' creates a file if it is not existing, and opening with 'r'
    allows modifying the database. Since 3.6 this emits deprecation warnings
    (bpo-21708). In future versions this will be an error.

    Proposed patch makes two changes:

    1. The index file no longer written if the database was not modified. This
      increases performance and adds a support of read-only files.

    2. A deprecation warning is raised when the index file is absent in 'r'
      and 'w' modes. In future versions this will be an error.

    May be the first change can be backported.

    ----------
    keywords: +patch
    stage: -> patch review
    type: behavior -> enhancement
    versions: +Python 3.7
    Added file: http://bugs.python.org/file45722/dbm_dumb_readonly.patch


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue28847\>


    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 2, 2016
    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Dec 2, 2016

    New changeset 0516f54491cb by Serhiy Storchaka in branch '2.7':
    Issue bpo-28847: dubmdbm no longer writes the index file in when it is not
    https://hg.python.org/cpython/rev/0516f54491cb

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 2, 2016

    The first part is committed in 2.7. I'll commit it in 3.5-3.7 after releasing 3.6.0.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Dec 3, 2016

    Serhiy: The Windows buildbots are having trouble removing read-only files. Maybe restore the write mode for the end of the test, or fix support.rmtree()? See <https://docs.python.org/3/library/shutil.html#rmtree-example\>.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Dec 3, 2016

    New changeset dc7c86de9e13 by Martin Panter in branch '2.7':
    Issue bpo-28847: Fix spelling
    https://hg.python.org/cpython/rev/dc7c86de9e13

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Dec 3, 2016

    http://buildbot.python.org/all/builders/AMD64%20Windows8%202.7/builds/9/steps/test/logs/stdio
    ======================================================================
    ERROR: test_readonly_files (test.test_dumbdbm.DumbDBMTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_dumbdbm.py", line 190, in test_readonly_files
        test_support.rmtree(dir)
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_support.py", line 289, in rmtree
        _rmtree(path)
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_support.py", line 245, in _rmtree
        _waitfor(_rmtree_inner, path, waitall=True)
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_support.py", line 199, in _waitfor
        func(pathname)
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_support.py", line 244, in _rmtree_inner
        _force_run(path, os.unlink, fullname)
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_support.py", line 194, in _force_run
        return func(*args)
    WindowsError: [Error 5] Access is denied: '@test_3796_tmp\\db.dat'
    
    ----------------------------------------------------------------------
    'test_dumbdbm' left behind directory '@test_3796_tmp'
    . . .
    test test_dumbdbm crashed -- <type 'exceptions.WindowsError'>: [Error 5] Access is denied: '@test_3796_tmp\\db.dat'
    Traceback (most recent call last):
      File "D:\buildarea\2.7.bolen-windows8\build\PCbuild\..\Lib\test\regrtest.py", line 948, in runtest_inner
        test_time = time.time() - start_time
      File "D:\buildarea\2.7.bolen-windows8\build\PCbuild\..\Lib\test\regrtest.py", line 899, in __exit__
        restore(original)
      File "D:\buildarea\2.7.bolen-windows8\build\PCbuild\..\Lib\test\regrtest.py", line 876, in restore_files
        test_support.rmtree(fn)
    . . .
    'test_dumbdbm' left behind directory '@test_3796_tmp' and it couldn't be removed: [Error 5] Access is denied: '@test_3796_tmp\\db.dat'
    Traceback (most recent call last):
      File "D:\buildarea\2.7.bolen-windows8\build\PCbuild\..\Lib\test\regrtest.py", line 1679, in <module>
        main()
      File "D:\buildarea\2.7.bolen-windows8\build\lib\contextlib.py", line 35, in __exit__
        self.gen.throw(type, value, traceback)
      File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_support.py", line 765, in temp_cwd
        rmtree(name)

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Dec 3, 2016

    New changeset cb4a892e9b66 by Serhiy Storchaka in branch '2.7':
    Try to fix test.test_support.rmtree() on Windows for fixing bpo-28847 tests.
    https://hg.python.org/cpython/rev/cb4a892e9b66

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 3, 2016

    Thanks Martin for pointing on this. Now buildbots are green.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Dec 7, 2016

    New changeset 0a74bc7ba462 by Serhiy Storchaka in branch '3.5':
    Issue bpo-28847: dbm.dumb now supports reading read-only files and no longer
    https://hg.python.org/cpython/rev/0a74bc7ba462

    New changeset 0c532bd28539 by Serhiy Storchaka in branch '3.6':
    Issue bpo-28847: dbm.dumb now supports reading read-only files and no longer
    https://hg.python.org/cpython/rev/0c532bd28539

    New changeset 2f59be67830c by Serhiy Storchaka in branch 'default':
    Issue bpo-28847: dbm.dumb now supports reading read-only files and no longer
    https://hg.python.org/cpython/rev/2f59be67830c

    New changeset a10361dfbf64 by Serhiy Storchaka in branch 'default':
    Issue bpo-28847: A deprecation warning is now emitted if the index file is missed
    https://hg.python.org/cpython/rev/a10361dfbf64

    @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
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants