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

bpo-14976: Reentrant simple queue #3346

Merged
merged 29 commits into from Jan 15, 2018
Merged

bpo-14976: Reentrant simple queue #3346

merged 29 commits into from Jan 15, 2018

Conversation

@pitrou
Copy link
Member

pitrou commented Sep 5, 2017

https://bugs.python.org/issue14976

TODO:

  • Add reentrancy test for C version
  • Update Windows build files
  • Add docs
@pitrou pitrou force-pushed the pitrou:simple_queue branch 2 times, most recently from e36a3c5 to ee5b4cb Sep 5, 2017
@pitrou pitrou force-pushed the pitrou:simple_queue branch from ee5b4cb to c0a087a Sep 5, 2017
pitrou added 2 commits Sep 5, 2017
@pitrou

This comment has been minimized.

Copy link
Member Author

pitrou commented Sep 5, 2017

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

Copy link
Contributor

rhettinger left a comment

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

@@ -8,16 +8,26 @@
from heapq import heappush, heappop
from time import monotonic as time

try:

This comment has been minimized.

Copy link
@rhettinger

rhettinger Sep 6, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@rhettinger

rhettinger Sep 6, 2017

Contributor

Can you swap in a deque() using PyObject_Call?

This comment has been minimized.

Copy link
@pitrou

pitrou Sep 6, 2017

Author Member

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?

This comment has been minimized.

Copy link
@rhettinger

rhettinger Sep 7, 2017

Contributor

Agreed. Go ahead and stick with the PyList calls.

@zooba
zooba approved these changes Sep 6, 2017
Copy link
Member

zooba left a comment

Approval for the Windows build files - they look fine.

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

This comment has been minimized.

Copy link
Member Author

pitrou commented Oct 23, 2017

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

@pitrou

This comment has been minimized.

Copy link
Member Author

pitrou commented Dec 19, 2017

@rhettinger could I have your feedback on this?

@pitrou pitrou requested a review from python/windows-team as a code owner Dec 19, 2017
Copy link
Member

gpshead left a comment

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

type2test = queue._PySimpleQueue


if _queue is not None:

This comment has been minimized.

Copy link
@gpshead

gpshead Jan 15, 2018

Member

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)

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

Sounds fair enough, will do.

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

This comment has been minimized.

Copy link
@gpshead

gpshead Jan 15, 2018

Member

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.

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

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.

Copy link
Member

serhiy-storchaka left a comment

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

}

/*[clinic input]
_queue.SimpleQueue.empty

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

You can use the bool return converter.

_queue.SimpleQueue.empty -> bool

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

Will do.

Py_ssize_t res;

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

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

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

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

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

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

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.

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

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.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

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

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

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

Inconsistent use of spaces in comments.

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

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

}

/*[clinic input]
_queue.SimpleQueue.qsize

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

You cause the Py_ssize_t return converter.

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

Will do.

}

/*[clinic input]
_queue.SimpleQueue.__init__

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

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

This comment has been minimized.

Copy link
@pitrou

pitrou Jan 15, 2018

Author Member

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

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 15, 2018

Member

For example tuple.__new__ or complex.__new__.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 15, 2018

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

@pitrou pitrou merged commit 94e1696 into python:master Jan 15, 2018
4 checks passed
4 checks passed
bedevere/issue-number Issue number 14976 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pitrou pitrou deleted the pitrou:simple_queue branch Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.