Support adding an established SOCK_STREAM connection to the reactor

[Jean-Paul Calderone]

Certain multi-process models want to be able to move established connections (TCP, UNIX, whatever) between processes. There aren't any public APIs for this now, so we should add one.

#5248 added IReactorSocket.adoptStreamPort. We should also add IReactorSocket.addConnection. It would have a similar signature to adopStreamPort (but likely without the socketType argument, since it would always have to be SOCK_STREAM) , but treat the file descriptor as a SOCK_STREAM socket which is already connected to a peer. The reactor would do whatever necessary to start processing events from that socket, just as it would do for a socket it accepted internally from a port it was managing.

See also #4387.

[therve]

(In [34512]) Branching to 'reactor-adopt-connection-5570'

[therve]

This is ready for review. I tried to follow what was going on with adoptStreamPort and everything to mimic the API, so I hope it fits in the grand plan.

[Jean-Paul Calderone]

Wow, cool, thanks!

1. Test coverage is lacking in a couple places: the UnsupportedAddressFamily case of adoptStreamConnection and the protocol is None case of _fromConnectedSocket. Note that tests for the cases similar to the former for adoptStreamPort are in twisted.internet.test.test_socket.
2. The rest of the test coverage seems pretty decent, but _testAdoptConnection harkens back to the bad old days of twisted.test.test_tcp a bit, I think.
   1. The test does a lot. Sets up a TCP server, runs a TCP client, exchanges data in both directions, d1 through d5? It's a bit of a challenge to follow because of all of this.
   2. At the same time, it doesn't do enough. eg, it doesn't demonstrate writeSequence behaves properly, it doesn't demonstrate correct unicode handling, it doesn't exercise abortConnection, etc. The direction I've been trying to take which addresses these last two points is to make tests fine grained and as re-usable as possible. Ideally what I'd like to suggest is creating a mixin that sets up a connection using this new API and a new subclass based on that mixin as well as another mixin defining tests for any ITCPTransport implementation. However, the tests are still pretty much a mess and there is no such ITCPTransport-test-mixin for you to use. But perhaps the new tests for this API could be factored this way? Then maybe we'll get there eventually.
3. A number of misuses of ipv6Skip seem to have been added to twisted.internet.test.test_tcp. The correct usage is like with test_portGetHostOnIPv6 - in other words, ipv6Skip is the value of the skip attribute, not a new string.
4. The new tests probably need to be skipped for reactors that don't implement IReactorSockets. See the requiredInterfaces attribute of ReactorBuilder.
5. I could go either way on adoptStreamConnection taking a factory or a protocol. Did you think about just taking a protocol at all? If not, what do you think of that idea?

Thanks again! Really exciting to see progress on this.

[therve]

Thanks for the review! I agree that the current tests are suboptimal, but I feel I'm missing some stuff to be able to move on:

• Do I need to exercise all the TCP related APIs just for that feature? (you mentioned unicode, abort, etc)
• If I want the tests to do less, what should I do? It seems I require at least a server and a client. Or should I just mock tons of stuff?

[therve]

I tried to move on with the idea of ITCPTransportTestMixin. I've handled most comments in the process, except the tests for protocol is None in _fromConnectedSocket, as I don't know how to check that.

I thought about factory VS protocol in in adoptStreamConnection, and I believe a factory is better as it allows to group the protocol instances under the factory, if you move all your file descriptors to another process for example.

[Jean-Paul Calderone]

Thanks. Sorry about not providing the clarification you asked for.

1. I guess it's worth pointing out for both adoptStreamConnection and adoptStreamPort that the file descriptor passed in is duplicated and the original can (and should) be closed by application code if it has no further use.
2. Some docstrings are missing from the tests
3. It would be good to use TCPCreator in these tests to make them more easily applicable to non-TCP cases, I guess. Look at the cases that set up a class creator attribute (since there's no endpoint for adoptStreamConnection I guess you can't do this everywhere, but it can certainly be done in some places).
4. Why does getConnectedClientAndServer accept a Deferred instead of making one itself and returning it? Running the reactor in a method that seems like a simple helper for getting a connection is surprising.
5. I wonder if reactormixins.runProtocolsWithReactor would simplify any of these tests.
6. I wonder why the tests still in WriteSequenceTests got left behind. Also, I actually liked the grouping of writeSequence tests into a test case named to reflect that. Maybe this organization could be preserved somehow? I do like the test implementation re-use you introduced in this version of the branch, I just hope we can keep writeSequence tests grouped together on a test case named more specifically than TCPTransportTestsBuilder. Transports are complex enough that I think it will be difficult to fit tests for all of a TCP transport into a single test case (names get awkward, mostly).

I'm still a bit up in the air about whether I like the factory argument. Your point about grouping makes sense, particularly since so many existing protocols depend on having a factory to work. The case that bothers me is the case where a connection with some state is being transferred. The factory seems like it would get in the way of properly initializing the protocol to handle, for exmaple, an IMAP4 connection that has already issued a dozen commands. It's certainly still possible with the factory (after all, just make the factory's protocol a lambda that returns an already constructed, initialized instance...) it just seems like it'll be awkward.

Thanks again.

[therve]

1. 2. 4. 6. Done!

3. 5. I think the generic aspect of getConnectedClientAndServer is better than trying to reuse more code just for the standard case. The lack of endpoint for the new adopt API is blocking using them everywhere, I think.

Regarding the factory, I agree we should do something. I see 3 possibilities:

• Add a new protocol argument, that you can pass instead of a factory.
• Add a documented helper which wraps an protocol instance with the lambda you're describing.
• Remove the factory argument, and let people manage grouping themselves.

The first would be my preferred choice.

[Jean-Paul Calderone]

Thanks!

I think the generic aspect of getConnectedClientAndServer is better than trying to reuse more code just for the standard case. The lack of endpoint for the new adopt API is blocking using them everywhere, I think.

Okay. I can live with that for now.

Can you elaborate on the first option for the factory issue a bit? Do you mean accepting either factory or protocol and using whichever is supplied? I kinda dislike that, particularly if the signature ends up as:

adoptStreamConnection(fileDescriptor, addressFamily, factory=None, protocol=None)

I'd feel slightly better about:

adoptStreamConnection(fileDescriptor, addressFamily, factoryOrProtocol)

But that probably just means I'm a bad person inside.

The second point is maybe a bit more attractive to me.

In particular, the documented helper should probably be:

Factory(protocol, *args, **kwargs)

Or perhaps twisted.internet.protocol._InstanceFactory should be tweaked and publicized somewhere.

[therve]

I've opened #5781 to tackle that problem. If it's satisfying we should be able to merge this branch.

[Jean-Paul Calderone]

Thanks!

1. Some skips needed on windows: ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/reactor-adopt-connection-5570
2. A minor conflict in test_tcp.py to be resolved
3. There was some logging in the previous versions of the unit tests. It would be helpful if it could be retained. (For example, logging when connections were established and when data was received in test_withoutWrite, but none is retained in the refactored version test_writeSequeceWithoutWrite).
4. Returning a Deferred from a ReactorBuilder test is weird. :/ It may not be a problem, but it at least seems like it might try to do something nonsensical. All ReactorBuilder-style tests make their own, new reactor and should be executing code that is driven by events from it. Return a Deferred asks trial to run the global reactor until that Deferred fires. Getting two different reactors involved in a test isn't going to be productive. Where the tests have a Deferred that might have a failure on it, I think it would be better to explicitly check it, not return it. (eg d.addErrback(list.append); if list: list[0].raiseException() sort of thing)

Thanks again for your work on this. Once you're happy with the code with respect to these points, please merge.

[therve]

(In [34981]) Branching to 'reactor-adopt-connection-5570-2'

[therve]

(In [34989]) Merge reactor-adopt-connection-5570-2

Author: therve Reviewer: exarkun Fixes: #5570

Add a new IReactorSocket.adoptStreamConnection method which allows registering an established connection in the reactor.