This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Queue.maxsize, __init__() accepts any value as maxsize
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 2.6
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, benjamin.peterson, rbp, rhettinger, zanella
Priority: low Keywords:

Created on 2008-02-20 21:09 by zanella, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
queue_maxsize.diff rhettinger, 2008-02-21 01:02 Patch for isinstance check in Py.26
queue_maxsize_2.diff amaury.forgeotdarc, 2008-02-21 08:39 Patch to allow any maxsize convertible to an int
queue_maxsize_3.diff zanella, 2008-02-21 13:06 Patch to allow any object with __int__ to be used, rhettinger's patch adds a test to "test/test_queue.py"
Messages (19)
msg62602 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-20 21:20
Queue.Queue(), accepts any value as the maxsize, example:

foo = Queue.Queue('j');
l = []; foo = Queue.Queue(l);
...

Shouldn't the value passed be checked on init :
isinstance(maxsize, int) ?
msg62603 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-20 22:06
This is probably a matter of style.  For the most part, library code
avoids isinstance() checks and lets the errors surface downstream. 

If a check gets added, we should probably also check that the value is
non-negative.  The checks should not be backported because it could
break Queue subclasses that rely on being able to pass in a non-int
value (like None or like a dynamic object that allows the maxsize to be
increased during the run).

I'm unclear why this got classified as a security issue rather than just
an RFE for Py2.6.
msg62604 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-20 22:15
Is there a problem with a dynamic object that changes the maxsize? I
think it might be better to ignore it, and let the client get exceptions
when their maxsize is compared.
msg62606 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-20 23:29
Firts: the security type was my error.

The method wich uses the maxsize:
"""
# Check whether the queue is full
def _full(self):
  return self.maxsize > 0 and len(self.queue) == self.maxsize
"""

@rhettinger: As per the documentation, negative values result on an
infinite Queue; well that AND will never be fulfilled with a negative
value anyway;

@gutworth: What I mean is that's "awkward", if you put an string for
example, it'll be the size of the string wich will be used on the
__cmp__ and on len(), but that's not explicit, or is it?

Example:

[zan@tails ~]$ python
Python 2.5.1 (r251:54863, Oct 30 2007, 13:54:11)
[GCC 4.1.2 20070925 (Red Hat 4.1.2-33)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C:
...   def __init__(self): pass;
...
>>> c = C()
>>> import Queue
>>> a = Queue.Queue(c)
>>> len(c)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: C instance has no attribute '__len__'
>>> a = Queue.Queue(c)
>>> a.put('q')
>>> a.get()
'q'
>>> a.put(1)
>>> a.put(2)
>>> a.put(3)
>>>
msg62609 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-21 02:00
Rafael, I agree that it's awkward, and I'm not against restricting the
maxsize to just something sane. However, I'm worried this (Raymond's
patch) will be too restrictive by not allowing dynamic changing of
maxsize. (Of course, you could just change the maxsize attribute of the
Queue, but that would require holding the mutex, too.)
msg62610 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-02-21 08:39
What about requiring maxsize to be convertible to an int?
This would allow dynamic objects, if they define an __int__ method.
I join a patch.
msg62620 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-21 12:56
I like it.
msg62621 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-21 13:06
@gutworth: Since one of the main uses of Queue is with threads, I think
it *really* should acquire the mutex before changing the maxsize;

@amaury.forgeotdarc: Your patch makes the point of allowing the size to
be changed at some other place (e.g.: an attribute of an instance passed
as the maxsize), but as stated above I think (am not an expert) the
mutex really should be held before the maxsize change, another point:
using an instance on your patch a call to Queue.maxsize would return
something like:
"""
Python 2.5.1 (r251:54863, Oct  5 2007, 13:36:32)
[GCC 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C:
...   def __init(self): self.n = 1;
...   def __int__(self): return int(self.n);
...
>>> c = C()
>>> import Queue
>>> q = Queue.Queue(c)
>>> q.maxsize
<__main__.C instance at 0xb7c341ac>
>>>                                        
"""
wich would force to have a get() to the maxsize attribute;

I have added a diff;
msg62624 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-21 15:34
Mine patch doesn't address the "hold the mutex before changing the
maxsize" guess it would then force a get()?
msg62629 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-21 16:58
It's probably best to close this as "won't fix".  Each of the patches 
limits the module or complicates it a bit.  I'm not sure there's even a 
real problem here.  My preference is to leave this code untouched.
msg62630 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-21 16:59
Maybe, we should change change the constructor to use self.maxsize =
int(maxsize). Then we can provide a set_maxsize method that will acquire
the mutex and preform the change. This is will restrict the type of
maxsize and allow for easy dynamic changing.
msg62632 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-21 17:05
Recommend closing this.  No need to muck-up a clean module to solve a 
non-problem.
msg62633 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-21 17:07
Raymond, are you referring to the int checking, my new method, or both?
msg62634 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-21 17:10
Both.  There is no issue here worth adding a new method.
msg62635 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-21 17:13
Ok. I agree that we shouldn't muddy the waters of Queue by checking for
int. (set_maxsize would be unneeded) Go ahead and close.
msg62735 - (view) Author: Rodrigo Bernardo Pimentel (rbp) (Python committer) Date: 2008-02-23 14:57
For what it's worth, I do think this is an issue. As it currently
stands, not only does the module silently accept invalid values, but the
mutex issue exists (and is also silently ignored) if an object returning
dynamic values is passed as maxsize. IMHO, the waters are already muddy,
it's just that the mud is blue so everything seems alright :)

I think zanella's patch is the way to go, even if it requires adding a
setter method.
msg62768 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-23 16:28
Many places in the stdlib accept values which are not valid. I believe
this is because the library trusts you to do the right thing in the name
of performance and cleaner, simpler code. IMO, adding a set_maxsize
method wouldn't be a sin, but Raymond (who is I'm sure wiser than me)
disagrees.
msg62800 - (view) Author: Rafael Zanella (zanella) Date: 2008-02-23 19:39
Just to exemplify:
"""
from threading import Thread
import time
import Queue

class C:
  def __int__(self):
    return 3
  
  #def __del__(self): print "collected..." # won't happen since q holds
a reference to it

c = C()
q = Queue.Queue(c)

# Not dynamic
print "maxsize: ", q.maxsize

# Not full() with instance
print c > 0
print len(q.queue) == q.maxsize

class T(Thread):
  def __init__(self, q):
    self._q = q
    Thread.__init__(self)
  
  def run(self):
    #For sme bizarre motive
    self._q.maxsize = 5

#Ends up being infinite most of the times
t = T(q)

for i in xrange(1000):
  q.put_nowait(i)
  if i == 1: # otherwise the "and len(self.queue) == self.maxsize" will fail
    t.start()
    time.sleep(1)

t.join()
"""

I guess rhettinger is right, there's no issue here, anyone that decides
to change the maxsize afterwards should know what is doing.

The only "possible" problem I'm able to see is someone passing an object
wich has __int__() and expecting it to be used.
msg62803 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-23 19:56
> The only "possible" problem I'm able to see is someone passing an object
wich has __int__() and expecting it to be used.
They should be explicit and say Queue(int(object_with__int__)).
History
Date User Action Args
2022-04-11 14:56:31adminsetgithub: 46402
2008-02-23 19:56:58benjamin.petersonsetmessages: + msg62803
2008-02-23 19:39:46zanellasetmessages: + msg62800
2008-02-23 16:28:08benjamin.petersonsetmessages: + msg62768
2008-02-23 14:57:42rbpsetnosy: + rbp
messages: + msg62735
2008-02-21 17:18:26rhettingersetstatus: open -> closed
resolution: wont fix
2008-02-21 17:13:42benjamin.petersonsetmessages: + msg62635
2008-02-21 17:10:47rhettingersetmessages: + msg62634
2008-02-21 17:07:10benjamin.petersonsetmessages: + msg62633
2008-02-21 17:05:07rhettingersetmessages: + msg62632
2008-02-21 16:59:48benjamin.petersonsetmessages: + msg62630
2008-02-21 16:58:53rhettingersetmessages: + msg62629
2008-02-21 15:34:56zanellasetmessages: + msg62624
2008-02-21 13:06:04zanellasetfiles: + queue_maxsize_3.diff
messages: + msg62621
2008-02-21 12:56:22benjamin.petersonsetmessages: + msg62620
2008-02-21 08:40:00amaury.forgeotdarcsetfiles: + queue_maxsize_2.diff
nosy: + amaury.forgeotdarc
messages: + msg62610
2008-02-21 02:00:29benjamin.petersonsetmessages: + msg62609
2008-02-21 01:02:53rhettingersetfiles: + queue_maxsize.diff
2008-02-20 23:29:27zanellasettype: security -> enhancement
messages: + msg62606
2008-02-20 22:15:09benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg62604
2008-02-20 22:06:26rhettingersetpriority: low
nosy: + rhettinger
messages: + msg62603
versions: + Python 2.6, - Python 2.5, Python 2.4
2008-02-20 21:20:40zanellasetmessages: + msg62602
2008-02-20 21:09:20zanellacreate