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

asyncore/asynchat patches #45089

Closed
josiahcarlson mannequin opened this issue Jun 13, 2007 · 16 comments
Closed

asyncore/asynchat patches #45089

josiahcarlson mannequin opened this issue Jun 13, 2007 · 16 comments
Assignees
Labels
stdlib Python modules in the Lib dir

Comments

@josiahcarlson
Copy link
Mannequin

josiahcarlson mannequin commented Jun 13, 2007

BPO 1736190
Nosy @gvanrossum, @brettcannon, @josiahcarlson, @giampaolo, @intgr
Files
  • async_patch2.patch: async sockets patch for 2.6
  • initiate_send.py
  • differences.diff
  • patch.diff: Updated patch (does not include test_suite and doc changes provided in first place since they're out of date)
  • full_async_patch.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 = 'https://github.com/josiahcarlson'
    closed_at = <Date 2008-07-03.18:14:57.713>
    created_at = <Date 2007-06-13.00:37:23.000>
    labels = ['library']
    title = 'asyncore/asynchat patches'
    updated_at = <Date 2009-03-28.12:22:27.291>
    user = 'https://github.com/josiahcarlson'

    bugs.python.org fields:

    activity = <Date 2009-03-28.12:22:27.291>
    actor = 'intgr'
    assignee = 'josiahcarlson'
    closed = True
    closed_date = <Date 2008-07-03.18:14:57.713>
    closer = 'josiahcarlson'
    components = ['Library (Lib)']
    creation = <Date 2007-06-13.00:37:23.000>
    creator = 'josiahcarlson'
    dependencies = []
    files = ['8050', '8917', '8918', '9587', '10196']
    hgrepos = []
    issue_num = 1736190
    keywords = ['patch']
    message_count = 16.0
    messages = ['52765', '52766', '52767', '52768', '57581', '58400', '63193', '64437', '66230', '66231', '66234', '66296', '66297', '67168', '69223', '74126']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'brett.cannon', 'josiahcarlson', 'alanmcintyre', 'giampaolo.rodola', 'djarb', 'svncodereview', 'intgr']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1736190'
    versions = ['Python 2.6']

    @josiahcarlson
    Copy link
    Mannequin Author

    josiahcarlson mannequin commented Jun 13, 2007

    There are a handful of outstanding asyncore/asynchat related bugs in the tracker. This patch seeks to handle all of the issues with the exception of windows-specific socket errors from 658749 .

    In particular, it takes pieces of patches 909005 and 1736101, to address some issues raised there, as well as the bugs: 539444, 760475, 777588, 889153, 953599, 1025525, and 1063924.

    In some cases where it could potentially handle fewer exceptions (see the send method), I have opted to handle more of them due to testing actually producing them.

    It adds test cases from patch 909005.

    It adds two new sample methods to asynchat.async_chat _get_data and _collect_incoming_data to be used as examples for handling incoming buffers.

    It removes the simple_producer and fifo classes from asynchat, but accepts work-alikes to the asynchat.async_chat.push_with_producer method.

    It further fixes an unreported violated invariant in asynchat.async_chat.handle_read() .

    It also produces a useful error message when asyncore.dispatcher.handle_expt_evt is called if the base handle_expt has not been overwritten.

    This patch should be applied to the trunk. The new methods, and removal of the two classes should not be included in patches to 2.5 maintenance (don't add the methods and don't remove the classes). Basically everything else should work as it did before (with better error handling), unless someone was monkey-patching asyncore.dispatcher.handle_expt .

    @josiahcarlson josiahcarlson mannequin self-assigned this Jun 13, 2007
    @josiahcarlson josiahcarlson mannequin added the stdlib Python modules in the Lib dir label Jun 13, 2007
    @josiahcarlson josiahcarlson mannequin self-assigned this Jun 13, 2007
    @josiahcarlson josiahcarlson mannequin added the stdlib Python modules in the Lib dir label Jun 13, 2007
    @giampaolo
    Copy link
    Contributor

    Good work. I would only rencommend to include documentation for asyncore.close_all function, actually not documented yet.

    @alanmcintyre
    Copy link
    Mannequin

    alanmcintyre mannequin commented Aug 8, 2007

    This patch removes two public--and documented--classes from asynchat (simple_producer and fifo). Even if the proposed changes to asynchat make them utterly worthless, I thought it was general policy to deprecate public items for a release or two before removing them. Is this the case even when they're as simple as these two classes?

    @josiahcarlson
    Copy link
    Mannequin Author

    josiahcarlson mannequin commented Aug 12, 2007

    We can leave those two classes in, even though I have never seen them used or subclassed. In Python 3.x, we can remove them without issue.

    @giampaolo
    Copy link
    Contributor

    The current implementation of asynchat.async_chat.initiate_send method
    doesn't look at what is specified in ac_out_buffer_size attribute which
    represents the buffer size of the outgoing data defaulting to a maximum
    of 4096 bytes to send in a single socket.send() call.

    Note that this only happens when sending the data by using a producer
    through the use of the push_with_producer method.
    This happens because while the older asynchat version used slicing for
    buffering:

    num_sent = self.send(self.ac_out_buffer[:obs]) # obs == ac_out_buffer_size

    ...the newer version just calls self.send using the entire data as
    argument without caring of what ac_out_buffer_size thinks about it:

    num_sent = self.send(first)

    What is specified in ac_out_buffer_size when using a producer is just
    ignored and the only way to have control over the outgoing data buffer
    is to operate directly on the producer.

    @giampaolo
    Copy link
    Contributor

    In attachment I provide a patch for fixing this last mentioned issue.
    It's a rewriting of initiate_send method which now looks at what is
    specified by ac_out_buffer_size attribute.

    @giampaolo giampaolo added type-security A security issue and removed type-security A security issue labels Jan 23, 2008
    @giampaolo
    Copy link
    Contributor

    I've discussed a lot with Josiah via e-mail and this is the updated
    version of the patch including a fix for the two issues raised before.
    This update has been needed also because the original patch has been
    out-dated by some commits after r53734 involving the test suite
    and the documentation, which I both took off this patch.
    I also re-added simple_producer and fifo classes in asynchat.py since it
    seems they are needed for passing tests.

    The test suite has passed on Windows XP using Python 2.6 alpha 1.
    I have also successfully run the test suite of a project of mine based
    on asynchat which includes over 40 tests.

    @giampaolo
    Copy link
    Contributor

    Are there news about this issue?

    @giampaolo
    Copy link
    Contributor

    In attachment the Josiah Carlson's patch who currently seems to have
    problems in accessing the bug tracker.
    It should address all the problems raised on python-dev related discussion:
    http://groups.google.com/group/python-dev2/browse_thread/thread/eec1ddadefe09fd8/40e79742231076b9?lnk=gst&q=asyncore#40e79742231076b9

    @brettcannon
    Copy link
    Member

    This is separate from bpo-1563, right?

    @giampaolo
    Copy link
    Contributor

    Yes.

    @gvanrossum
    Copy link
    Member

    FWIW, I've added Giampaolo's latest patch to Rietveld:
    http://codereview.appspot.com/744

    Review comments added there should automatically be CC'ed here.

    @svncodereview
    Copy link
    Mannequin

    svncodereview mannequin commented May 5, 2008

    Dear report@bugs.python.org,

    New code review comments by gvanrossum@gmail.com have been published.
    Please go to http://codereview.appspot.com/744 to read them.

    Message:
    (This is mostly a test of the bug/rietveld integration.)

    Details:

    http://codereview.appspot.com/744/diff/1/22
    File Doc/library/asyncore.rst (right):

    http://codereview.appspot.com/744/diff/1/22#newcode226
    Line 226: A file_dispatcher takes a file descriptor or file object
    along with an optional
    Mind keeping the line length under 80 chars?

    Issue Description:
    http://bugs.python.org/issue1736190

    Sincerely,

    Your friendly code review daemon (http://codereview.appspot.com/).

    @svncodereview
    Copy link
    Mannequin

    svncodereview mannequin commented May 21, 2008

    Dear GvR, report,

    New code review comments by josiah.carlson have been published.
    Please go to http://codereview.appspot.com/744 to read them.

    Message:

    Details:

    http://codereview.appspot.com/744/diff/1/22
    File Doc/library/asyncore.rst (right):

    http://codereview.appspot.com/744/diff/1/22#newcode226
    Line 226: A file_dispatcher takes a file descriptor or file object
    along with an optional
    On 2008/05/05 22:02:22, GvR wrote:

    Mind keeping the line length under 80 chars?

    No problem. Any other comments?

    Issue Description:
    http://bugs.python.org/issue1736190

    Sincerely,

    Your friendly code review daemon (http://codereview.appspot.com/).

    @josiahcarlson
    Copy link
    Mannequin Author

    josiahcarlson mannequin commented Jul 3, 2008

    Committed to trunk a bit ago, will be in 3.0 this weekend.

    @josiahcarlson josiahcarlson mannequin closed this as completed Jul 3, 2008
    @josiahcarlson josiahcarlson mannequin closed this as completed Jul 3, 2008
    @giampaolo
    Copy link
    Contributor

    This issue should be closed.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants