Change the error (EPOLLHUP, EPOLLERR) checking code so that it can
handle the case where they are both set, rather than only checking
the cases where one or the other is set. This allows epollreactor
to properly detect and report connections which are lost uncleanly
and also are not being polled for input events.

The test case for this is slightly convoluted for a number of
reasons:

gtkreactor cannot pass the new test, and I am not fixing that
reactor in this branch. The test will skip when that reactor
is in use. #2833 was created as a result of this in order to
be able to remove the broken gtkreactor eventually.

If a file descriptor is being polled for neither read nor write
events, it will not be polled for hang-up events either. This
is a known issue, #1662. When it is fixed, the test will need
to be updated.

In order to exercise the case where the event reported from epoll
is only HUP|ERR (specifically, not IN), the test pauses one of
the involved transports. However, due to #1780, it must use a
timing hack to accomplish this. Once that issue is resolved,
it should be possible to remove this hack.

I've looked quickly at this, and it doesn't seem very hard, but I see a problem: if we fix pauseProducing to just work in connectionMade, we might expect to break some stuff (I haven't check in Twisted itself).

I have one proposition, which would be to add a semantic to connectionMade: the returned value means whether you want to start reading immediately or not (with a special check for None, meaning True). Writing it, I realize that's not really a good idea, but I don't have another for now :). We might just as well fix pauseProducing, I don't know.

Regarding breaking existing code, do you have a particular kind of breakage in mind? The obvious case - where code calls pauseProducing in connectionMade and expects it to have no effect - doesn't seem like a reasonable expectation for code to have; that is, it strikes me as clearly buggy behavior, not behavior changes to which need to follow the backwards compatibility guidelines.

I agree with exarkun; the fact it doesn't work is a bug, there's no way a reasonable person would expect it *not* to work. Also, code that depends on the fact calling a random method is a noop at a given point seems unlikely.

flags kind of suck, since often combinations of different flags are unexpected or untested. but, if a flag is the way to implement this, then I don't really object.

maybe you could describe what you encountered that makes you think a new flag is the way to solve this, though? someone might be able to suggest a simpler solution (I kind of expected this to be more or less resolved by re-ordering some framework and application code).

It'd be good to have an explicit unit test just for this case too, though.

I can't really think of a blackbox test that could be written for this. It's a test for the actual TCP transport, so it has to use the real TCP transport classes. That means it's subject to whatever decisions the platform TCP implementation wants to make. That means it's hard to be sure the data won't be delivered after pauseProducing is called (as opposed to it just not having been delivered yet). We could probably write something that was almost correct - send a lot of data, wait a second, see if any of it got delivered. Most (maybe even all extant) TCP implementations would deliver it fast enough for the test to notice it and fail within a pretty short period of time. Still, that's pretty crummy.

A whitebox test for this could just check the reactor to make sure the transport isn't being monitored for events. Is that better than a time-sensitive test? I'm not sure. I think so. But then it needs an implementation for each reactor. Maybe that's alright too, though.

If there were a public API for checking what events a... thing... (file descriptor? I dunno) is being monitored for, then that whitebox test would become a blackbox test, though.

I'd probably name isMonitoredForReadinghasReader to mirror addReader and removeReader.

test_pauseProducingInConnectionMade is good, but maybe not quite as complete as it could be. An assertion that the monitored state is not tampered with after connectionMade (really makeConnection) returns would be a nice addition.

I'm not really sure what we want to do about adding new methods to IReactorFDSet. It's probably fine, but of course if there are any third-party implementations they'll be incomplete with this change.

I'd probably name isMonitoredForReadinghasReader to mirror addReader and removeReader.

Done.

test_pauseProducingInConnectionMade is good, but maybe not quite as complete as it could be. An assertion that the monitored state is not tampered with after connectionMade (really makeConnection) returns would be a nice addition.

Done.

I'm not really sure what we want to do about adding new methods to IReactorFDSet. It's probably fine, but of course if there are any third-party implementations they'll be incomplete with this change.

That brings something: iocpreactor doesn't implement IReactorFDSet, so we can skip the test easily.

Skip for PauseProducingTestCase.test_pauseProducingInConnectionMade would be nicer outside the test method (like the one for ReactorFDTestCase).

The cfreactor implementation of getReaders and getWriters might be simpler if they used keys (several other implementations do that, so I'm not sure if cfreactor was overlooked or intentionally excluded)?

It'd be nice to merge the branch forward and try it on buildbot. Right now there's a bunch of failures that are probably due to other stuff which has since been fixed.

If you're feeling very adventurous, then I think ReactorFDTestCase would be well expressed as a parameterized test case which uses a newly created reactor instead of the global one. testSuite works well enough now that we can start writing these, I think, and it would be good if we did. On the other hand, I don't think there's yet a public API for making all the necessary TestCase instances, so it might be better to have this as a separate task rather than part of this one.