async_chat.push() can trigger handle_error(). undocumented.

[jjdmol]

BPO	1370380
Nosy	@josiahcarlson, @giampaolo, @intgr

^{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 = None
closed_at = <Date 2009-03-31.18:17:17.297>
created_at = <Date 2005-11-30.21:18:48.000>
labels = ['docs']
title = 'async_chat.push() can trigger handle_error(). undocumented.'
updated_at = <Date 2009-03-31.18:17:17.296>
user = 'https://bugs.python.org/jjdmol'

bugs.python.org fields:

activity = <Date 2009-03-31.18:17:17.296>
actor = 'josiahcarlson'
assignee = 'none'
closed = True
closed_date = <Date 2009-03-31.18:17:17.297>
closer = 'josiahcarlson'
components = ['Documentation']
creation = <Date 2005-11-30.21:18:48.000>
creator = 'jjdmol'
dependencies = []
files = []
hgrepos = []
issue_num = 1370380
keywords = []
message_count = 9.0
messages = ['26959', '26960', '26961', '26962', '26963', '26964', '26965', '26966', '62393']
nosy_count = 4.0
nosy_names = ['josiahcarlson', 'jjdmol', 'giampaolo.rodola', 'intgr']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1370380'
versions = ['Python 2.4']

[jjdmol]

suppose you have a socket handled by your asynchat.async_chat class
and want to send a message (call push()). push() calls initiate_send(),
which can call handle_error() if an error occurs. however, once
handle_error() returns, the control is passed back to push(), which has
no return value thus cannot signal the success or failure to the code
that did the push(). i.e. if we have code like

foo()
s.push(data)
bar()

the following is executed in case of an error:

foo()
s.push(data)
s.handle_error()
bar()

this created an obscure bug, as the bar() assumed the send() always
succeeds, and also cannot check directly by the result of push() if it
did. this creates the false illusion push() can never fail.

to avoid this, handle_error() can set a flag, which can be checked after
the push(). however, this is an ugly hack of course.

PS: send() can easily fail. suppose we're reading from 2 sockets: A and
B, and send messages to both when data is read from either one. if B
gets disconnected and A has data ready, both will be in the set of
readible sockets returned by select(). if A's data is handled before B's,
and A sends a message to B, this will cause the send to fail as B is
disconnected. your app wont know B failed, because this is processed
after the data from A is processed.

[josiahcarlson]

Logged In: YES
user_id=341410

Not a bug. The control for deciding how to handle an error
in a connection is defined by the instance method
handle_error(). If you would like custom error handling in
your instances, perhaps you should subclass async_chat...

import asynchat

class MyAsyncChat(asynchat.async_chat):
    error = 0
    def handle_error(self):
        self.error = 1
        asynchat.async_chat.handle_error(self)
    def push(self, data):
        self.error = 0
        asynchat.async_chat.push(self, data)
        return self.error

Also, the fact that async_chat.push() ends up trying to send
data, is, in my opinion, a misfeature in implementation.
Under certain circumstances it can increase data throughput,
but it removes the standard asyncore.loop/poll() from the
control flow.

[jjdmol]

Logged In: YES
user_id=516066

Push() sending data may indeed be the source of the problems here. There
should be no problems with delaying the write until the next select() will tell
the socket is writable.

My current work around is indeed subclassing as you describe. However, it
seemed that such a thing would be exactly that: a work around in order to
obtain expected behaviour.

So a suggestion: push() should either not try to send, or should communicate
back to its caller when an error occurs. Having an error handler set an error
code to check is so 80s, but that could be just me :)

Maybe push() not sending is the nicer of the two solutions. There is little
reason why it can't wait till the next select() call returns the socket as
writable.

If nothing is changed, the documentation should contain that handle_error()
can be triggered even though one is only adding stuff to a FIFO buffer (or so
the description of push() makes it seem). Reading the docs doesn't prepare
you for the control flow as I first described. In all the other handle_error
invocations, handle_error() isn't called from within your code unless you raise
exceptions in your part of the handle_read/write/accept/connect code. Even
in asyncore, send() gives you an exception instead, creating an inconsistency
with asynchat's push(), as both can fail.

[josiahcarlson]

Logged In: YES
user_id=341410

Here's a subclass that doesn't send on push...

class MyAsyncChat(asynchat.async_chat):
    def push (self, data):
        self.producer_fifo.push (simple_producer (data))

That's it.

And according to my reading of asynchat and asyncore, while
actually trying to send data during a push() may trigger the
handle_error() method to be called, by default, the
handle_error() method logs the error and closes the socket.
Errors don't seem to propagate out of push().

[jjdmol]

Logged In: YES
user_id=516066

Oh I fully agree that its easy to write a workaround. And that the error doesn't
propagate out of push(). It's just the question whether this is desired
behaviour and expected behaviour after reading the documentation? If not,
either the code or documentation ought to be modified.

Right now, you wouldn't tell from the docs that push() will try to send data
(and thus fail, which is the only moment handle_error() can occur in the
middle your code if you catch exceptions yourself).

This creates all sorts of obscure behaviour:

class MyAsyncProtocol(asynchat.async_chat):
  def handle_connect(self):
    self.foo = some_value_I_calculate
    self.push( "hello!" )

class MyLoggingAsyncProcotol(MyAsyncProtocol):
  def handle_connect(self):
    MyAsyncProtocol.handle_connect(self)
    print "Connection established with foo value %d" % self.foo
  def handle_error(self):
    print "Connection lost"

Could produce the following output:
Connection lost
Connection established with foo value 42

I wouldnt expect this from the documentation: push() adds data to the output
buffer, but the docs dont say or imply it can fail and thus trigger handle_error
().

A simple solution would be to put all push()es at the end of the function so
that no more code is executed except maybe handle_error(). But as the
example above shows, this is not always possible. Another solution would be
one of your workarounds.

If only the docs would be adapted, it would still bother me to have to do the
above for any but the most trivial of applications. But once the docs warns me
for this behaviour, I at least know it from the start :)

So, do you feel docs/code should be changed, and if so, which one?

[josiahcarlson]

Logged In: YES
user_id=341410

If anything should be changed, I would say the docs. Though
I disagree with the send during a call to push(), for most
users, it makes sense, and convincing the current maintainer
that removing the send on push for the sake of async purity
would be difficulty (practicality beats purity).

I believe that adding a note which reads something like the
following would be sufficient.
"The default push() implementation calls the underlying
socket send() (to increase data throughput in many
situations), and upon error, will call handle_error(). This
may cause unexpected behavior if push() is used within a
subclassed handle_connect()."

[jjdmol]

Logged In: YES
user_id=516066

Ok thanks. The example with handle_connect() is just one out of many btw: in
general, don't run code after push() which uses or assumes data which
handle_error() cleans up.

There are indeed cases in which initiate_send() in a push() is practical. Aside
from throughput issues, sending heartbeat messages or streaming data from
another thread using a timer doesn't work without initiate_send, because the
data wouldn't be sent until select() in the main thread feels like returning.

[josiahcarlson]

Logged In: YES
user_id=341410

Manipulating the same socket in two threads is a fool's
business. If you want to force select to return, you should
have a shorter timeout for select, or a loopback socket in
which both ends are in the same process, where you can send
some data, which causes the select to be triggered for a
read event.

[giampaolo]

If this won't going to be fixed/modified I suggest to close this report
since it's frozen from year 2005.