dbm.dumb should be consistent when the database is closed

[PCManticore]

BPO	19385
Nosy	@ncoghlan, @PCManticore, @JimJJewett, @serhiy-storchaka
Files	dbm_dumb.patch dbm_dumb1.patch dbm_dumb1.patch: add space in test issue19385.patch issue19385_1.patch issue19385_speed.patch issue19385_speed_2.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/ncoghlan'
closed_at = <Date 2014-05-28.15:53:06.063>
created_at = <Date 2013-10-25.06:38:41.007>
labels = ['type-feature', 'library']
title = 'dbm.dumb should be consistent when the database is closed'
updated_at = <Date 2014-05-28.15:53:06.062>
user = 'https://github.com/PCManticore'

bugs.python.org fields:

activity = <Date 2014-05-28.15:53:06.062>
actor = 'serhiy.storchaka'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2014-05-28.15:53:06.063>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2013-10-25.06:38:41.007>
creator = 'Claudiu.Popa'
dependencies = []
files = ['32347', '32449', '32453', '34337', '35016', '35241', '35348']
hgrepos = []
issue_num = 19385
keywords = ['patch']
message_count = 29.0
messages = ['201209', '201819', '201823', '201824', '201900', '206050', '206058', '206069', '206094', '206151', '206183', '206184', '206185', '206416', '213069', '213840', '217091', '217093', '217097', '217116', '217216', '218435', '218440', '218441', '218444', '219083', '219277', '219281', '219283']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'Arfrever', 'Claudiu.Popa', 'python-dev', 'Jim.Jewett', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19385'
versions = ['Python 3.4', 'Python 3.5']

[PCManticore]

This problem occurred in bpo-19282. Basicly, dbm.dumb is not consistent with dbm.gnu and dbm.ndbm when the database is closed, as seen in the following:

>>> import dbm.dumb as d
>>> db = d.open('test.dat', 'c')
>>> db.close()
>>> db.keys()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tank/libs/cpython/Lib/dbm/dumb.py", line 212, in keys
    return list(self._index.keys())
AttributeError: 'NoneType' object has no attribute 'keys'
>>>

vs

>>> import dbm.gnu as g
>>> db = g.open('test.dat', 'c')
>>> db.close()
>>> db.keys()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
_gdbm.error: GDBM object has already been closed
>>>

Attaching a patch.

[ncoghlan]

Patch looks good to me, I'll apply it when I apply bpo-19282.

[serhiy-storchaka]

Additional checks can slowdown the code. It doesn't matter in some operations, but not on __len__, __contains__, etc. EAFP approach is to catch AttributeError and raise appropriate dbm.dumb.error exception.

[serhiy-storchaka]

Yet one nitpick. I think that closing check should be after argument type check and key encoding.

[PCManticore]

Here's the new version which addresses your last comment. Regarding the first issue, I don't believe that the result will be as readable (but I agree with you that it will be better). For instance, items will probably look like this:

try:
return [(key, self[key]) for key in self._index.keys()]
except AttributeError:
raise dbm.dumb.error(...) from None

but will look totally different for other __len__:

try:
return len(self._index)
except TypeError:
raise dbm.dumb.error(...) from None.

We could catch TypeError only for dunder methods though and for the rest of the methods check the value of _index before access.

[PCManticore]

Serhiy, what's the status for this? Should this be targeted for 3.5?

[serhiy-storchaka]

Regarding the first issue, I don't believe that the result will be as readable (but I agree with you that it will be better).

Yes, perhaps performance of the dbm.dumb module is not worst this cost. Are you have any benchmarking results?

The patch LGTM.

Should this be targeted for 3.5?

I think yes.

[PCManticore]

Here's a benchmark:

• with current patch:

# ./python -S -m timeit -n 10000 -s 'import dbm.dumb as dbm; d=dbm.open("x.dat", "c");d.close()' 'try:' ' len(d)' 'except OSError:' ' pass'
10000 loops, best of 3: 1.78 usec per loop

• using
  try:
  return len(self._index)
  except TypeError:
  raise error('...')

# ./python -S -m timeit -n 10000 -s 'import dbm.dumb as dbm; d=dbm.open("x.dat", "c");d.close()' 'try:' ' len(d)' 'except OSError:' ' pass'
10000 loops, best of 3: 3.27 usec per loop

Now this seems odd, maybe catching + reraising an exception has a greater overhead than a func call and checking if an attribute is None. Anyway, I agree that dbm.dumb is not performance critical.

[Arfrever]

IMHO it is a bug fix, not a new feature, and could be applied in 3.3 and 3.4.

[serhiy-storchaka]

Now this seems odd, maybe catching + reraising an exception has a greater overhead than a func call and checking if an attribute is None.

Yes, of course, catching + reraising an exception is costly. But when an exception is not raised, this is cheap. Usually len() is called for open database.

IMHO it is a bug fix, not a new feature, and could be applied in 3.3 and 3.4.

Hmm. If consider dbm.dumb as separated module, this is perhaps a new feature. But if consider it as an implementation of the dbm protocol, it should conform to other dbm modules, and this is a bugfix. What would you say Nick?

[ncoghlan]

I think it's definitely still OK to fix this in 3.4, but I think it's borderline enough to avoid including in the last ever 3.3 release. Changing exception types always introduces a little backwards compatibility risk, so it's something I lean towards only doing in new feature releases, even in cases like this where the old exception was clearly not the best choice.

[Arfrever]

in the last ever 3.3 release.

msg204533 suggests that there will be >=2 bug fix releases in 3.3 branch (3.3.4 soon and 3.3.5 around release of 3.4.0).