multiprocessing.Queue raises AssertionError

[JosephSiddall]

BPO	22872
Nosy	@pitrou, @berkerpeksag, @serhiy-storchaka, @zvyn, @applio, @ZackerySpytz, @tirkarthi
PRs	bpo-22872: multiprocessing.Queue's put() and get() now raise ValueError if the queue is closed #9010
Files	issue22872.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 2018-10-18.12:55:10.917>
created_at = <Date 2014-11-14.17:34:48.511>
labels = ['3.8', 'type-feature', 'library']
title = 'multiprocessing.Queue raises AssertionError'
updated_at = <Date 2018-10-18.12:55:10.916>
user = 'https://bugs.python.org/JosephSiddall'

bugs.python.org fields:

activity = <Date 2018-10-18.12:55:10.916>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-10-18.12:55:10.917>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2014-11-14.17:34:48.511>
creator = 'Joseph.Siddall'
dependencies = []
files = ['39700']
hgrepos = []
issue_num = 22872
keywords = ['patch']
message_count = 7.0
messages = ['231164', '245291', '251178', '258737', '313790', '327644', '327979']
nosy_count = 10.0
nosy_names = ['pitrou', 'jnoller', 'sbt', 'berker.peksag', 'serhiy.storchaka', 'zvyn', 'Joseph.Siddall', 'davin', 'ZackerySpytz', 'xtreak']
pr_nums = ['9010']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22872'
versions = ['Python 3.8']

[JosephSiddall]

putting something in Queue(multiprocessing.Queue) after closing it raises an AssertionError.
Getting something out of a Queue after closing it raises an OSError.
I expected both scenarios to raise the same exception.

To Reproduce:

>>> from multiprocessing import Queue
>>> q = Queue()
>>> q.close()
>>> q.put("ok")
Traceback (most recent call last):
...
AssertionError


>>> from multiprocessing import Queue
>>> q = Queue()
>>> q.close()
>>> q.get()
Traceback (most recent call last):
...
OSError: handle is closed

[zvyn]

Here is the trivial patch for that :)

[applio]

The proposed patch would potentially break existing code which anticipates the current behavior. The potential negative impact means this particular proposed patch is not viable.

Choices forward include: (1) better documenting the existing, established implementation behavior (perhaps having the assert provide a more informative message), or (2) altering the behavior in a new release version (perhaps 3.6 when it comes) and documenting that change appropriately there.

As to what sort of exception we should expect in this situation, unfortunately comparing to the queue module's Queue does not help guide expectations much since it does not have need of a close().

Of the two above options, I'm more inclined towards the first. Does @Joseph.Siddall have other motivations that helped motivate this suggested improvement?

[berkerpeksag]

See bpo-5001 for more general cleanup in multiprocessing.

[...] (2) altering the behavior in a new release version (perhaps 3.6 when it comes) and documenting that change appropriately there.

+1. We can also document AssertionError in older Python versions.

[pitrou]

The expected behaviour here would be to raise ValueError, as on other closed objects:

>>> s = io.StringIO()
>>> s.close()
>>> s.write("")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: I/O operation on closed file

[serhiy-storchaka]

New changeset 0461704 by Serhiy Storchaka (Zackery Spytz) in branch 'master':
bpo-22872: multiprocessing.Queue's put() and get() now raise ValueError if the queue is closed. (GH-9010)
0461704

[serhiy-storchaka]

I don't think we need to document AssertionError in older Python versions. This is an implementation detail. We don't document all exceptions that can be raised with improper use of the API. Just don't do this.

In addition, using the assert statement for validating user input or the object state which depends on user actions is a bad style. We shouldn't encourage this by documenting it as a behavior of the stdlib. It was a bug, and it is fixed now.