bpo-14976: Reentrant simple queue by pitrou · Pull Request #3346 · python/cpython

pitrou · 2017-09-05T15:56:01Z

https://bugs.python.org/issue14976

TODO:

Add reentrancy test for C version
Update Windows build files
Add docs

pitrou · 2017-09-05T16:18:21Z

Hmm, would someone want to update the Windows build files for the new _queue extension for me? @zooba, @pfmoore, @zware perhaps?

…e_queue

rhettinger

When the docs are made, the reentrancy guarantee should be listed as a C-implementation detail.

rhettinger · 2017-09-06T17:08:31Z

Lib/queue.py

 from heapq import heappush, heappop
 from time import monotonic as time

+try:


The usual way to incorporate the C extension is to put a "from _queue import SimpleQueue"

rhettinger · 2017-09-06T17:17:05Z

Modules/_queuemodule.c

+    self = (simplequeueobject *) type->tp_alloc(type, 0);
+    if (self != NULL) {
+        self->weakreflist = NULL;
+        self->lst = PyList_New(0);


Can you swap in a deque() using PyObject_Call?

It would replace all the simple PyList C API calls with more cumbersome method calls through the PyObject API. Also, since generic method calls can allocate tuples, they might release the GIL (though I haven't checked for sure). Do you think it's worth it?

Agreed. Go ahead and stick with the PyList calls.

zooba

Approval for the Windows build files - they look fine.

pitrou · 2017-10-23T16:33:16Z

@rhettinger, what should be the way forward here? Do you have further requests on this PR?

pitrou · 2017-12-19T15:03:13Z

@rhettinger could I have your feedback on this?

…e_queue

gpshead

overall good, a couple comments to think about but nothing worth blocking this.

gpshead · 2018-01-15T20:51:54Z

Lib/test/test_queue.py

+    type2test = queue._PySimpleQueue
+
+
+if _queue is not None:


rather than a big conditional indented class implementation, why not use @unittest.skipIf(_queue is None, 'No _queue module found') on the class? (your load time type2test assignment can be made conditional or done at runtime in a setUp() method so it won't run when skipped)

Sounds fair enough, will do.

gpshead · 2018-01-15T20:57:10Z

Lib/queue.py

+class _PySimpleQueue:
+    '''Simple, unbounded FIFO queue.
+
+    This pure Python implementation is not reentrant.


What is the point of the pure python version existing at all of it's raison d'être isn't possible? Even on PyPy if they took this module and haven't implemented a _queue equivalent it would do the wrong thing for code that wants to use SimpleQueue for reentrant API reasons.

I know its presence is debattable, but providing a pure Python version is current policy. Besides, being reentrant is documented as an implementation detail, not an intrinsic property of the API.

serhiy-storchaka

The size of the internal list shouldn't be included in the result of __sizeof__.

serhiy-storchaka · 2018-01-15T21:02:33Z

Modules/_queuemodule.c

+}
+
+/*[clinic input]
+_queue.SimpleQueue.empty


You can use the bool return converter.

_queue.SimpleQueue.empty -> bool

serhiy-storchaka · 2018-01-15T21:06:30Z

Modules/_queuemodule.c

+    Py_ssize_t res;
+
+    res = _PyObject_SIZE(Py_TYPE(self));
+    res += _PySys_GetSizeOf(self->lst);


Since the list is visited in tp_traverse, its size shouldn't be include in the result of __sizeof__.

Hmm... what does tp_traverse have to do with it?

sys.getsizeof() is a low-level tool that returns the only size of the object, without subobjects. A high-level tool will calls sys.getsizeof() recursively for the tree of objects using gc.get_referents(). gc.get_referents() is build by using tp_traverse. If __sizeof__ includes the size of objects exposed in tp_traverse, it will be counted twice.

That's a fair point, even though any non-trivial __sizeof__ defined in pure Python would have the same problem (see for example the pure-Python OrderedDict.__sizeof__).

I'll make the change.

OrderedDict.__sizeof__ is incorrect. In general pure Python classes shouldn't define __sizeof__ (as well as __basicsize__ or __dictoffset__).

serhiy-storchaka · 2018-01-15T21:08:02Z

Modules/_queuemodule.c

+    0,                                  /*tp_iter*/
+    0,                                  /*tp_iternext*/
+    simplequeue_methods,                /*tp_methods*/
+    0,                                  /* tp_members */


Inconsistent use of spaces in comments.

I will happily let others reformat if they're irritated by inconsistent spaces in comments.

serhiy-storchaka · 2018-01-15T21:08:42Z

Modules/_queuemodule.c

+}
+
+/*[clinic input]
+_queue.SimpleQueue.qsize


You cause the Py_ssize_t return converter.

serhiy-storchaka · 2018-01-15T21:20:36Z

Modules/_queuemodule.c

+}
+
+/*[clinic input]
+_queue.SimpleQueue.__init__


I don't think this method is needed. Instead use Argument Clinic for the __new__ method.

Is there an example somewhere of using Argument Clinic for a __new__ method?

For example tuple.__new__ or complex.__new__.

bedevere-bot · 2018-01-15T21:28:11Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pitrou added 2 commits September 3, 2017 13:35

Add queue.SimpleQueue type

9c3431b

Add C version of SimpleQueue

5d2f5cc

pitrou requested a review from rhettinger as a code owner September 5, 2017 15:56

the-knights-who-say-ni added the CLA signed label Sep 5, 2017

bedevere-bot added the awaiting merge label Sep 5, 2017

Docstrings and comments

d3e890c

pitrou force-pushed the simple_queue branch 2 times, most recently from e36a3c5 to ee5b4cb Compare September 5, 2017 16:04

Simplify some code away.

c0a087a

pitrou force-pushed the simple_queue branch from ee5b4cb to c0a087a Compare September 5, 2017 16:04

pitrou added 2 commits September 5, 2017 18:05

Fix expectation in test

fa67c79

Updated generated files

a58acf0

pitrou added 9 commits September 5, 2017 19:01

Try to support --without-threads

0f86f5c

Fix EINTR-handling logic

3d9c709

Merge branch 'master' of https://github.com/python/cpython into simpl…

5620970

…e_queue

Add qsize(), empty() and __sizeof__()

a76c041

Add optional block and timeout args to put(), for compatibility.

bc07e44

Add put_nowait() and get_nowait() compatibility methods

f8fe713

Add reentrancy tests

3e1b27d

Merge branch 'master' of https://github.com/python/cpython into simpl…

a250d56

…e_queue

Merge branch 'master' of https://github.com/python/cpython into simpl…

3d761a8

…e_queue

rhettinger reviewed Sep 6, 2017

View reviewed changes

Try to add _queue module to Visual Studio build files

fdb5379

zooba approved these changes Sep 6, 2017

View reviewed changes

pitrou added 5 commits September 6, 2017 20:01

Try to fix compilation on MSVC

f1d731d

Change import idiom (and fix __all__)

6f66a52

Add a test that references to items are not kept longer than they should

8847e03

Add docs

1773aff

Merge branch 'master' into simple_queue

57e6675

pitrou changed the title ~~[WIP] bpo-14976: Reentrant simple queue~~ bpo-14976: Reentrant simple queue Sep 7, 2017

Add NEWS blurb

4041847

Merge branch 'master' of https://github.com/python/cpython into simpl…

16cbd05

…e_queue

pitrou requested a review from a team as a code owner December 19, 2017 15:06

pitrou added 3 commits December 19, 2017 16:10

Remove obsolete WITH_THREAD guard

c75ccf6

Merge branch 'master' into simple_queue

c3aeb82

Update clinic file

c963121

gpshead approved these changes Jan 15, 2018

View reviewed changes

serhiy-storchaka requested changes Jan 15, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jan 15, 2018

pitrou added 3 commits January 15, 2018 22:44

Address review comments

3337e24

Use Argument Clinic for __new__

3646e99

Remove __sizeof__ implementation as suggested by Serhiy

0d03ff5

serhiy-storchaka approved these changes Jan 15, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 15, 2018

pitrou merged commit 94e1696 into python:master Jan 15, 2018

bedevere-bot removed the awaiting merge label Jan 15, 2018

pitrou deleted the simple_queue branch January 15, 2018 23:27

Uh oh!

Conversation

pitrou commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Sep 5, 2017

Uh oh!

rhettinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 23, 2017

Uh oh!

pitrou commented Dec 19, 2017

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pitrou commented Sep 5, 2017 •

edited

Loading