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.

Author josiahcarlson
Recipients
Date 2007-01-07.04:42:22
SpamBayes Score
Marked as misclassified
Message-id
In-reply-to
Content
Many of the changes in the source provided by klimkin in his most recent revision from February 27, 2005 seek to solve certain problems in an inconsistent or incorrect way.  Some of his changes (or variants thereof) are worthwhile.  I'll start with my issues with his asyncore changes, then describe what I think should be added from them.

For example, in his updated asyncore.py, the list of sockets is first shuffled randomly, then sorted based on priority.  Assuming that one ignored priorities for a moment, if there were more sockets than the max sockets for the platform, then due to the limitations of randomness, there would be no guarantees that all sockets would get polled.  Say, for example, that one were using windows and were running close to the actual select file handle limit (512 in Python 2.3) with 500 handles, you would skip 436 of the sockets *this pass*.  In 10 passes, there would have been 100 sockets that were never polled.  In 20 passes, there would still be, on average, 20 that were never polled.  So this "randomization" step is the wrong thing to do, unless you actually make multiple select calls for each poll() call.  But really, select is limited by 512, and I've run it with 500 without issue.

The priority based sorting has much of the same problems, but it is even worse when you have nontrivial numbers of differing priorities, regardless of randomization or not.

The max socket limit of 64 on Windows isn't correct.  It's been 512 since at least Python 2.3 .  And all other platforms being 65536?  No.  I've had some versions of linux die on me at 512, others at 4096, but all were dog slow beyond 500 or so.  It's better to let the underlying system raise an exception for the user when it fails and let them attempt to tune it, rather than forcing a tuning that may not be correct.


The "pending read" stuff is also misdirected.  Assuming a non-broken async client or server, either should be handling content as it comes it, dispatching as necessary.  See asynchat.collect_incoming_data() and asynchat.found_terminator() for examples.

The idispatcher stuff seems unnecessary.


Generally speaking, it seems to me that there are 3 levels of abstraction going on:
1) handle_*_event(), called by poll, poll2, etc.
2) handle_*(), called by handle_*_event(), user overrides, calls other handle_*() and *() methods
3) *() (aka recv, send, close, etc.), called by handle_*(), generally left alone.

Some of your code breaks the abstraction and has items in layer 2 call items in layer 1, which then call items in layer 2 again.  This seems unnecessary, and breaks the general downward calling semantic (except in the case of errors returned by layer 3 resulting in layer 2 handle_close() calls, which is the proper method to call).


There are, according to my reading of the asyncore portions of your included module, a few things that may be worthy for inclusion into the Python standard library are:

* A variant of your changes to close_all(), though it should proceed in closing everything unless a KeyboardInterrupt, SystemExit, or ExitNow exception is raised.  Socket errors should be ignored, because we are closing them - we don't care about their error condition.

* Checking sockets for socket error via socket.getsockopt() .

* A variant of your .close() implementation.

* The CONNRESET, etc., stuff in the send() and recv() methods, but not the handle_close_event() replacements, stick with handle_close() .

* Checking for KeyboardInterrupt and SystemExit inside the poll functions.

* The _closed_socket class and initialization.

All but the last of the above, I would consider to be bugfixes, and if others agree that these are reasonable changes, I'll write up a patch against trunk and 2.5 maintenance.  The last change, while I think would be nice, probably shouldn't be included in 2.5 maintenance, though I think would be fine for the trunk.
History
Date User Action Args
2007-08-23 15:32:38adminlinkissue909005 messages
2007-08-23 15:32:38admincreate