classification
Title: dumbdbm should not commit if in read mode
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Jonathan Ng, martin.panter, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-12-01 03:50 by Jonathan Ng, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
dbm_dumb_readonly.patch serhiy.storchaka, 2016-12-01 12:41 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (13)
msg282133 - (view) Author: Jonathan Ng (Jonathan Ng) Date: 2016-12-01 03:53
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.
msg282158 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-01 11:29
Do you have concrete example when not ignoring an OSError in _update causes an issue? This is needed for writing tests.
msg282166 - (view) Author: Jonathan Ng (Jonathan Ng) Date: 2016-12-01 12:23
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
'''
msg282167 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-01 12:41
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 (issue21708). 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.
msg282170 - (view) Author: Jonathan Ng (Jonathan Ng) Date: 2016-12-01 12:48
#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
> (issue21708). 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>
> _______________________________________
>
msg282218 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-02 05:59
New changeset 0516f54491cb by Serhiy Storchaka in branch '2.7':
Issue #28847: dubmdbm no longer writes the index file in when it is not
https://hg.python.org/cpython/rev/0516f54491cb
msg282219 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-02 06:00
The first part is committed in 2.7. I'll commit it in 3.5-3.7 after releasing 3.6.0.
msg282264 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-12-03 04:04
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>.
msg282265 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-03 04:05
New changeset dc7c86de9e13 by Martin Panter in branch '2.7':
Issue #28847: Fix spelling
https://hg.python.org/cpython/rev/dc7c86de9e13
msg282266 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-12-03 04:12
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)
msg282269 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-03 05:58
New changeset cb4a892e9b66 by Serhiy Storchaka in branch '2.7':
Try to fix test.test_support.rmtree() on Windows for fixing issue28847 tests.
https://hg.python.org/cpython/rev/cb4a892e9b66
msg282271 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-03 06:59
Thanks Martin for pointing on this. Now buildbots are green.
msg282602 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-07 09:11
New changeset 0a74bc7ba462 by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #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 #28847: A deprecation warning is now emitted if the index file is missed
https://hg.python.org/cpython/rev/a10361dfbf64
History
Date User Action Args
2017-03-31 16:36:22dstufftsetpull_requests: + pull_request962
2016-12-07 09:12:22serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-12-07 09:11:46python-devsetmessages: + msg282602
2016-12-03 06:59:23serhiy.storchakasetmessages: + msg282271
2016-12-03 05:58:18python-devsetmessages: + msg282269
2016-12-03 04:12:56martin.pantersetmessages: + msg282266
2016-12-03 04:05:14python-devsetmessages: + msg282265
2016-12-03 04:04:17martin.pantersetnosy: + martin.panter
messages: + msg282264
2016-12-02 06:00:47serhiy.storchakasetmessages: + msg282219
versions: - Python 2.7
2016-12-02 05:59:08python-devsetnosy: + python-dev
messages: + msg282218
2016-12-02 05:41:50serhiy.storchakasetassignee: serhiy.storchaka
versions: + Python 2.7, Python 3.5, Python 3.6
2016-12-01 12:48:37Jonathan Ngsetmessages: + msg282170
2016-12-01 12:41:50serhiy.storchakasetfiles: + dbm_dumb_readonly.patch
versions: + Python 3.7
messages: + msg282167

keywords: + patch
type: behavior -> enhancement
stage: patch review
2016-12-01 12:23:05Jonathan Ngsetmessages: + msg282166
2016-12-01 11:29:04serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg282158
components: + Library (Lib)
2016-12-01 03:53:47Jonathan Ngsetmessages: + msg282133
title: dumbdbm should not commit if -> dumbdbm should not commit if in read mode
2016-12-01 03:50:06Jonathan Ngcreate