When using asynchat.async_chat channel, and connection is refused,
asyncore fail incorrectly.

First, instead of ECONNREFUSED, you get EPIPE, second, you also get a
EBADF exception, and finally, the channel handle_close is called twice.

The problem is the way asyncore detect a connect event. On the first
read event, asyncore set the connected flag to true, and invoke the
channel handle_connect method. Typically, the channel will try to push
something to the remote endpoint at this point. Since "connected" is
true, push() will try to send to the socket, and fail with EPIPE,
because the socket is not connected.

The socket error in push will invoke the channel handle_error, which
typically will call handle_close to close the socket.
handle_connect_event will return without any error, because the error
was handled inside push.

Now asyncore will invoke the channel handle_read method, which fails
with bad file descriptor, since the channel closed the socket on the
previous error.

How should this work correctly?

1. We want to get a connection refused error in this case
2. The failure should cause only single exception
3. The channel should be close once

I added these test to test_asyncore.py, and all of them fail with
current code.

Turns out that single line change fix all the failing tests: set the
"connected" state after the call to handle_connect, just like it used to
be in 2.5.

With this change, when the channel try to push something to the remote
endpoint, it will keep it in the fifo, since connected is false. This is
also correct, since we will know that we are connected only after
handle_read is called. In handle_read, we fail with ECONNREFUSED and
close the channel.

The fix is tested currently only on Mac OS X 10.5. I have seen this
issue also on Ubuntu 9.04.

The patch is tested with release26-maint and trunk.

Tested on Ubuntu Linux 9.04.

The tests will probably fail on Windows, since connection refused is
detected trough handle_expt_event, and not in hadnle_read_event. I hope
someone on Windows will fix this :-)

The first fix reverted to 2.5 behavior, when self.connected is false when
handle_connect is called. This behavior is little stupid - why call
handle_connect if the socket is not really connected?

This fix ensure that handle_connect is called only if the socket is
connected - just like it is done in handle_write_event. Now it does not
matter when you set the connected flag, before or after handle_connect.

Tested in trunk on Mac OS X.

This version fix also handle_expt_event, so connection refused error
should be handled in the same way also on Windows.

The attached patch cleans up the remnants of the "handle_expt is for
exceptions", which isn't the case, as well as makes the "connection
refused" fix actually work on Windows. Nirs, could you verify this on
*nix?

I have a big problem with asyncore_fix_refused.patch - it assumes that a
dispatcher has a socket attribute, which can be used with t getsockopt().
This is true in the default dispatcher class implemented in asyncore, but
wont work with file_dispatcher, or 3rd party dispatcher class.

The framework should let you drop in your dispatcher class, that will
implement a minimal interface - handle_x_event, writable, readable etc.

What are the issues on Windows with asyncore-handle-connect-event-3.patch
on Windows?

Firstly, it expects that handle_expt_event() is for handling exceptional
conditions. This is not the case. handle_expt_event() is meant for
handling "OOB" or "priority" data coming across a socket. FTP and some
other protocols use this. I forgot to fix it earlier, which is why it's
making it into this patch.

Secondly, I pulled the part that was inside handle_expt_event() that was
being used to find the exception and pulls it out into _exception(),
removing the previous behavior (wrt to the broken API), and replacing it
with something that is cleaner and more correct (assuming sockets).

To respond to it being an issue that the object has a socket with a
getsockopt(), I can fix it to handle the AttributeError case.

Would you be willing to try this out given my explanation as to why I
changed your patch?

I'll check the patch this week.

The asyncore framework has low level events - handle_read_event, handle_write_event and
handle_expt_event - these events are not used for reading, writing and OOB - they are just
responsible to call the high level events.

The high level events - handle_connect, handle_accept, handle_read, handle_write,
handle_close and handle_expt should be used only for specific events.

I don't see any problem in checking for errors in handle_expt_event, it works just like
handle_read_event, that calls handle_connect.

This design allow you do replace the dispatcher with your own dispatcher class,
implementing only the low level events.

Originally, handle_expt_event() was described as "handles OOB data or
exceptions", but over-using handle_expt_event() as an error/close
handler is a bad idea. The function asyncore.readwrite() (called by
asyncore.poll2()) does the right thing WRT handle_expt_event(), which it
makes sense to apply to the standard select-based asyncore.poll().
That's what this does (in addition to fixing the close case that you
pointed out).

In terms of "only implementing low-level stuff", this is still the case.
You still only need to implement handle_(), not handle__event() . But
now, handle_expt_event() isn't written to do more than it should have
been doing in the first place.

I've updated the patch to include semantics for actually handling OOB
data, which I've verified by using a slightly modified pyftpdlib (remove
the socket option calls to set socket.SO_OOBINLINE) and it's tests on
both Windows and Ubuntu 8.04 (I also ran the full Python test suite on
my Ubuntu install, and any failures were obviously not asyncore/asynchat
related).

I tested asyncore_fix_refused-3.patch on Mac OS X 10.5 - all asyncore
and asynchat tests pass.

There is one minor issue - _exception calls the non existing
handle_close_event instead of handle_close.

However, looking again at the code I think that it is ugly and wrong -
handle_expt_event should handle the low level expt event called from
select, allowing third party dispatcher to override the behavior as
needed.

Another issue - lately, a new event was added - handle_connect_event -
this is wrong! there is no such low level event. handle_connect is a
high level event, implied by first read or write on the connecting
socket. This event will break 3rd party dispatcher that does not
implement it, and is not documented.

I was wrong about handle_connect_event - it is called only from the
dispatcher, so it will not break 3rd party dispatcher.

This is asyncore-fix-refused-3.patch with some fixes:

1. Call handle_close instead of non exiting handle_close_event
2. Remove unneeded handle_close_event in test classes
3. Revert removal of handle_expt_event in test classes - not clear why it
   was removed in the previous patch.

Tested on Mac OS X 10.5.

handle_expt_event was removed in the test classes because it is no
longer being used by any of the tests. None of them send OOB data (also
known as priority data), so handle_expt_event should never be called.
When I have a chance to compare your patch to mine (Monday, likely),
I'll comment then.

In terms of "handle_expt_event should handle the low level expt event
called from select", what you don't seem understand is that and "expt
event" is not an exception event, it's a "we got OOB data" event, and
with the patches, it is called at *exactly* the time it should be: when
there is OOB data. It used to be mistakenly called in the select loop
whenever any sort of non-normal condition happened on the socket, which
was a serious design flaw, and lead to significant misunderstanding
about the purpose of the method.

With the _exception() call as-is, it now behaves like the
asyncore.poll2() function, which is the right thing.

handle_expt is documented to be called when there is OOB data. However,
handle_expt_event is not documented, and according the framework design
as I see it, it simply means "socket has exceptional condition" when
select returns. On unix, this means there is oob data, and on Windows,
it means "there is some error".

This works exactly the same for handle_read_event and handle_write_event

• they may be called on connection refused error. Checking for errors in
  handle_expt_event is the right thing to do, and allow you to avoid the
  ugly checks and double try..except in _exception.

If you want handle_foo_event to be called only on foo events, then they
will not have anything to do except calling handle_foo. This is actually
the case now in handle_expt_event. I don't see any advantage of this
design change.

What's the current status of this issue?
I think some of the problems raised at the time this was opened should have been fixed in meantime, like checking for errors in handle_expt_event and handle_connect_event (bpo-2944).

The handle_close being called twice problem is still there though, as shown in bpo-4690.

Not sure what other problems remain.

Has maintenance of asyncore effectively ceased owing to tulip/asyncio or are outstanding problems here, if any, still considered valid?

@victor as you've been looking at other asyncore/chat issues can you look at this please?

The latest patch was written 5 years ago, it's probably a huge work to rebase it. Giampaolo wrote that the code changed a lot since this time (especially code handling errors).

asyncore-fix-refused-4.patch catchs EBADF. To me, it looks like a bug: if you get a EBADF error, we already made something wrong before.

The patch doesn't call obj.handle_expt_event() anymore in _exception() if obj is not a socket. It don't understand why. I don't understand neither why _exception() now raises a new socket error instead of calling obj.handle_expt_event().

All these changes are non trivial. I don't feel able to review the patch.

Sorry, I'm not interested to invest time on this deprecated module. It's time to move forward, to asyncio, and so I close this issue.