Opened 7 years ago
Closed 7 years ago
#5570 enhancement closed fixed (fixed)
Support adding an established SOCK_STREAM connection to the reactor
Reported by: | Jean-Paul Calderone | Owned by: | therve |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Branch: |
branches/reactor-adopt-connection-5570-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | therve |
Description (last modified by )
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.
Change History (15)
comment:1 Changed 7 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 7 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 7 years ago by
Owner: | set to therve |
---|
comment:4 Changed 7 years ago by
Author: | → therve |
---|---|
Branch: | → branches/reactor-adopt-connection-5570 |
comment:5 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
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.
comment:6 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
Wow, cool, thanks!
- Test coverage is lacking in a couple places: the
UnsupportedAddressFamily
case ofadoptStreamConnection
and theprotocol is None
case of_fromConnectedSocket
. Note that tests for the cases similar to the former foradoptStreamPort
are intwisted.internet.test.test_socket
. - The rest of the test coverage seems pretty decent, but
_testAdoptConnection
harkens back to the bad old days oftwisted.test.test_tcp
a bit, I think.- The test does a lot. Sets up a TCP server, runs a TCP client, exchanges data in both directions,
d1
throughd5
? It's a bit of a challenge to follow because of all of this. - At the same time, it doesn't do enough. eg, it doesn't demonstrate
writeSequence
behaves properly, it doesn't demonstrate correctunicode
handling, it doesn't exerciseabortConnection
, 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 anyITCPTransport
implementation. However, the tests are still pretty much a mess and there is no suchITCPTransport
-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.
- The test does a lot. Sets up a TCP server, runs a TCP client, exchanges data in both directions,
- A number of misuses of
ipv6Skip
seem to have been added totwisted.internet.test.test_tcp
. The correct usage is like withtest_portGetHostOnIPv6
- in other words,ipv6Skip
is the value of the skip attribute, not a new string. - The new tests probably need to be skipped for reactors that don't implement
IReactorSockets
. See therequiredInterfaces
attribute ofReactorBuilder
. - 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.
comment:7 Changed 7 years ago by
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?
comment:8 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
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.
comment:9 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
Thanks. Sorry about not providing the clarification you asked for.
- I guess it's worth pointing out for both
adoptStreamConnection
andadoptStreamPort
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. - Some docstrings are missing from the tests
- 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 classcreator
attribute (since there's no endpoint foradoptStreamConnection
I guess you can't do this everywhere, but it can certainly be done in some places). - Why does
getConnectedClientAndServer
accept aDeferred
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. - I wonder if
reactormixins.runProtocolsWithReactor
would simplify any of these tests. - I wonder why the tests still in
WriteSequenceTests
got left behind. Also, I actually liked the grouping ofwriteSequence
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 keepwriteSequence
tests grouped together on a test case named more specifically thanTCPTransportTestsBuilder
. 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.
comment:10 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
- 2. 4. 6. Done!
- 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.
comment:11 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
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.
comment:12 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
I've opened #5781 to tackle that problem. If it's satisfying we should be able to merge this branch.
comment:13 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
Thanks!
- Some skips needed on windows: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/reactor-adopt-connection-5570
- A minor conflict in test_tcp.py to be resolved
- 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 versiontest_writeSequeceWithoutWrite
). - Returning a
Deferred
from aReactorBuilder
test is weird. :/ It may not be a problem, but it at least seems like it might try to do something nonsensical. AllReactorBuilder
-style tests make their own, new reactor and should be executing code that is driven by events from it. Return aDeferred
asks trial to run the global reactor until thatDeferred
fires. Getting two different reactors involved in a test isn't going to be productive. Where the tests have aDeferred
that might have a failure on it, I think it would be better to explicitly check it, not return it. (egd.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.
comment:14 Changed 7 years ago by
Branch: | branches/reactor-adopt-connection-5570 → branches/reactor-adopt-connection-5570-2 |
---|
(In [34981]) Branching to 'reactor-adopt-connection-5570-2'
comment:15 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [34512]) Branching to 'reactor-adopt-connection-5570'