Opened 3 years ago

Closed 16 months ago

#5574 enhancement closed fixed (fixed)

Add support for SOCK_DGRAM to IReactorSocket

Reported by: exarkun Owned by: rwall
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/udp-port-from-fd-5574-4
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

#5248 added support for some taking some existing SOCK_STREAM listening sockets and adding them to the reactor.

A similar feature would be support for adding an existing SOCK_DGRAM socket to the reactor.

This would probably be exposed as a new IReactorSocket method.

Change History (28)

comment:1 Changed 18 months ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

comment:2 Changed 18 months ago by rwall

  • Author set to rwall
  • Branch set to branches/udp-port-from-fd-5574

(In [38323]) Branching to 'udp-port-from-fd-5574'

comment:3 Changed 18 months ago by rwall

I've taken most of the changes made that were made for adoptStreamPort in #5248 and applied them to the udp.

TODO:

  • Fix twistedchecker problems
  • Add maxPacketSize to interface
  • Add tests for maxPacketSize
  • Write a script to do some functional testing with systemd
  • check why udp.Port.startListening doesn't have the same addressFamily check and _resolveIPv6 call as tcp.Port

Build Results look promising:

comment:4 Changed 18 months ago by rwall

  • Keywords review added
  • Owner rwall deleted
  • Status changed from assigned to new

Ready for review in log:branches/udp-port-from-fd-5574

  1. This isn't very original work, I've just copied and adapted the changes in #5248 for UDP sockets.
  2. Added an IReactorSocket.adoptDatagramPort method - I considered calling it adoptDGRAMPort but thought it looked ugly.
  3. Refactored the test_udp.py module so that tests can be reused for both newly created UDP sockets and adopted sockets.
  4. Added a verifyClass test for IReactorSocket because I'd forgotten to add the maxPacketSize argument to the IReactorSocket. (although I expected that to be built into the ReactorBuilder since you have to specify "requiredInterfaces").
  5. There is a verifyClass test for MemoryReactor which is why I added adoptDatagramPort to that - even though I don't use it in this branch.
  6. Improved the docstrings in listenUDP and udp.Port.init.
  7. tcp.Port.startListening has special handling of IPv6 address "scope identifiers" but udp.Port does not. Is it relevant for UDP? Probably a separate ticket anyway.
                if self.addressFamily == socket.AF_INET6:
                    addr = _resolveIPv6(self.interface, self.port)
                else:
                    addr = (self.interface, self.port)
    
  8. There was already a lot of duplication in test_socket module, and I've made it worse. I considered factoring out the common tests and implementing mixins containing getListeningPort for the DGRAM and STREAM sockets - let me know if that makes sense in this branch.

Build Results:

  1. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/udp-port-from-fd-5574
  2. documentation builder seems to have highlighted a bunch of existing errors instead of giving a "new errors" list. I think they can be ignored.
  3. twistedchecker should ignore the missing @param etc in TestMixin classes - I think they can be ignored.
  4. I deliberately didn't document PosixBaseReactor.adoptDatagramPort so that the full API docs can be inherited from IReactorSocket.

-RichardW.

comment:5 follow-up: Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall
  1. _bindSocket needs a docstring.
  2. The comment on _preexistingSocket seems redundant with the docstring for it.
  3. In SocketUDPMixin, I'm concerned with portSocket.fileno() raising, in a finally block. Is there any reason you can't just call .close() there?
  4. I'm sad that you need to specify DatagramProtocol instead of IDatagramProtocol, but I guess this isn't the place to fix it.
    • A related point is that difference between connected and unconnected datagram sockets. But I guess we are consitent at refering to the later just undecorated.
  5. I considered factoring out the common tests and implementing mixins containing getListeningPort for the DGRAM and STREAM sockets - let me know if that makes sense in this branch.

I think that makes sense. I don't see a lot of duplication there currently, so I think refactoring things to avoid more duplication is sensible.

  1. I don't think the verifyClass tests belong in test_socket. Probably test_posixbase`?

Please address the above, and resubmit for review.

comment:6 Changed 17 months ago by rwall

  • Branch changed from branches/udp-port-from-fd-5574 to branches/udp-port-from-fd-5574-2

(In [38845]) Branching to 'udp-port-from-fd-5574-2'

comment:7 in reply to: ↑ 5 Changed 17 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Thanks for the review Tom.

I've merged forward and addressed some of your code review points in
log:branches/udp-port-from-fd-5574-2

Some further comments and responses below:

Replying to tom.prince:

  1. _bindSocket needs a docstring.

Done.

  1. The comment on _preexistingSocket seems redundant with the docstring for it.

Done. And moved the neighbouring comments to API docs.

  1. In SocketUDPMixin, I'm concerned with portSocket.fileno() raising, in a finally block. Is there any reason you can't just call .close() there?

I think it's part of the test.

To ensure that adoptXXXPort doesn't close the port.

Exarkun originally introduced it in r34014.

Shall I wrap it in assertNotRaises?

  1. I'm sad that you need to specify DatagramProtocol instead of IDatagramProtocol, but I guess this isn't the place to fix it.

I looked for existing tickets or discussion of such an interface but
can't find anything.

https://encrypted.google.com/search?hl=en&q=site%3Atwistedmatrix.com%20IDatagramProtocol
No results found for site:twistedmatrix.com IDatagramProtocol

  • A related point is that difference between connected and
  • unconnected datagram sockets. But I guess we are consitent at
  • refering to the later just undecorated.

Connected UDP is something you do later from the DatagramProtocol. I
don't think that makes any difference to the adoptDatagramPort API.

Or were you just commenting on the proposed IDatagramProtocol.

I'll leave you to create a ticket for IDatagramProtocol if you like.

5.

I considered factoring out the common tests and implementing

mixins containing getListeningPort for the DGRAM and STREAM sockets

  • let me know if that makes sense in this branch. I think that makes sense. I don't see a lot of duplication there currently, so I think refactoring things to avoid more duplication is sensible.

I tried refactoring test_socket, but there are a number of subtle
differences between the Stream and DGRAM tests, and the resulting
abstract tests were difficult to read eg

  • adoptMethod = 'adoptStreamPort' / adoptDatagramPort
  • factoryOrProtocol = ServerFactory / DatagramProtocol
  • if self.adoptMethod == 'adoptStreamPort': socket.listen(1)
  • if self.adoptMethod == 'adoptStreamPort': socket.accept() else: socket.recvfrom()

So I've abandoned that idea.

  1. I don't think the verifyClass tests belong in test_socket. Probably test_posixbase`?

I've removed it. I added it because I was confused by a failing test
which turned out to be bug in my test.

Please address the above, and resubmit for review.

Thanks for the review.

-RichardW.

comment:8 Changed 17 months ago by tom.prince

  1. I'm sad that you need to specify DatagramProtocol instead of IDatagramProtocol, but I guess this isn't the place to fix it.
    • A related point is that difference between connected and
    • unconnected datagram sockets. But I guess we are consitent at
    • refering to the later just undecorated.

Connected UDP is something you do later from the DatagramProtocol. I don't think that makes any difference to the adoptDatagramPort API.

Well, that is true of UDP, but isn't necesarily true of connecting to a unix socket, for example. Or, having been passed a UDP socket that is already connected, I guess.

Or were you just commenting on the proposed IDatagramProtocol.

Yes.

  1. I don't think the verifyClass tests belong in test_socket. Probably test_posixbase`?

I've removed it. I added it because I was confused by a failing test which

turned out to be bug in my test.
I wasn't suggesting you remove it, just move it.

comment:9 follow-up: Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall
  1. Please add a test that PosixBase implements IReacotrSocket (or perhaps a ReactorBuilder test that verifies everything claiming to implement IReactorSocket actually implements it properly.
    • This isn't directly related to this ticket, but as you mentionedit helped track down an issue in this ticket.
  2. In the docstring for _bindSocket: "Reponsible for" doesn't convey any useful information (in the same way that "Check that" or "Test that" doesn't, in test docstrings).
  3. This should also support unix sockets. Please file a ticket for this.

Please merge after addressing the above points.

comment:10 Changed 16 months ago by rwall

  • Branch changed from branches/udp-port-from-fd-5574-2 to branches/udp-port-from-fd-5574-3

(In [38905]) Branching to 'udp-port-from-fd-5574-3'

comment:11 in reply to: ↑ 9 Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Ready for another review log:branches/udp-port-from-fd-5574-3

Replying to tom.prince:

  1. Please add a test that PosixBase implements IReacotrSocket (or perhaps a ReactorBuilder test that verifies everything claiming to implement IReactorSocket actually implements it properly. - This isn't directly related to this ticket, but as you mentionedit helped track down an issue in this ticket.

Done. I added a new ReactorBuilder test to test_socket.

  1. In the docstring for _bindSocket: "Reponsible for" doesn't convey any useful information (in the same way that "Check that" or "Test that" doesn't, in test docstrings).

Done.

  1. This should also support unix sockets. Please file a ticket for this.

Raised #6594 and linked to it in the IReactorSocket doc string.

I also removed a link to adopt stream connections ticket which has
been closed.

Please merge after addressing the above points.

I'd like another review to check that I've done the connected socket
detection correctly.

Thanks.

-RichardW.

comment:12 Changed 16 months ago by rwall

Hmm. It looks like there are a few build errors:

  1. OSX returns a different error number (57) for Socket is not connected. Perhaps there's no ned to check the error number: http://buildbot.twistedmatrix.com/builders/osx10.6-py2.6-select/builds/3461/steps/select/logs/problems
  2. The IReactorSocket tests don't seem to be skipped on Windows reactors: Not sure what to do about that. http://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/2724/steps/select/logs/problems

comment:13 follow-up: Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall

The IReactorSocket tests don't seem to be skipped on Windows reactors: Not sure what to do about that.

This appears due to the fact that ReactorBuilder only checks requiredInterfaces in .buildReactor. I'm not sure how best to handle this.

Perhaps move the logic to when tests are built? This wouldn't work if a reactor used directlyProvides, but I don't think any of the existing ones do. Get some input from exarkun or glyph on this.

OSX returns a different error number (57) for Socket is not connected. Perhaps there's no ned to check the error number.

Perhaps handling connected sockets should be addressed in a seperate ticket?

In any case, I'm not entirely convinced that that the same function should handle both.

  • Depending on which case it is, the function will expect a DatagramProtocol or a ConnectedDatagramProtocol.
  • In the unix case, there is already connectUNIX vs. listenUNIX with both different transport interfaces.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Ready for another review in log:branches/udp-port-from-fd-5574-3

Replying to tom.prince:

This appears due to the fact that ReactorBuilder only checks
requiredInterfaces in .buildReactor. I'm not sure how best to
handle this.

Perhaps move the logic to when tests are built? This wouldn't work
if a reactor used directlyProvides, but I don't think any of the
existing ones do. Get some input from exarkun or glyph on this.

Yes I think you're right.

I've modified just that test to check that
ReactorBuilder.reactorFactory implements IReactorSocket before
attempting to verify it.

OSX returns a different error number (57) for Socket is not connected. Perhaps there's no ned to check the error number.

Perhaps handling connected sockets should be addressed in a seperate
ticket?

I agree. I reverted those changes and raised #6594.

In any case, I'm not entirely convinced that that the same function should handle both.

  • Depending on which case it is, the function will expect a
  • DatagramProtocol or a ConnectedDatagramProtocol.
  • In the unix case, there is already connectUNIX vs. listenUNIX
  • with both different transport interfaces.

Looks like ConnectedDatagramProtocol would only be expected / allowed
when adopting for AF_UNIX sockets.

class ConnectedDatagramProtocol(DatagramProtocol):
    """Protocol for connected datagram-oriented transport.

    No longer necessary for UDP.
    """

comment:15 in reply to: ↑ 14 Changed 16 months ago by rwall

Replying to rwall:

Replying to tom.prince:

Perhaps move the logic to when tests are built? This wouldn't work
if a reactor used directlyProvides, but I don't think any of the
existing ones do. Get some input from exarkun or glyph on this.

Yes I think you're right.
I've modified just that test to check that
ReactorBuilder.reactorFactory implements IReactorSocket before
attempting to verify it.

I asked glyph about it and I don't think he's convinced that we need to verifyClass _and_ verifyObject.

I don't have any good reason for adding both. I think I was just trying to be thorough.

So anyway I've now removed test_implementer.

comment:16 follow-up: Changed 16 months ago by tom.prince

This looks good. One thing I missed, before, is that there isn't any narrative documentation for this. It would be nice if some could be added before this gets merged.

Sorry.

Other than that, this is good to merge.

comment:17 Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall

comment:18 in reply to: ↑ 16 Changed 16 months ago by rwall

Replying to tom.prince:

This looks good. One thing I missed, before, is that there isn't any narrative documentation for this. It would be nice if some could be added before this gets merged.

I've added a short example (r39002) based on the code in:

You can view the styled page here:

How much we want to document this interface?

Presumably we'd prefer people to use an IDatagramEndpoint #4471 which I might have a go at next.

Also note that there isn't any documentation or examples of t.i.endpoints.AdoptStreamEndpoint or IReactorSocket.adoptStreamPort either.

comment:19 Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

comment:20 Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall

Thanks.

That looks like a nice self contained example.

I suspect that in practice, somebody would be using this with systemd, or something like this.

Until we get datagram endpoints, you could use it with systemd with something like (untested)

from twisted.python.systemd import listenFDs
from twisted.internet import reactor
fd = listenFDs.fromEnvironment().inheritedDescriptors()[0]
port = reactor.adoptDatagramPort(
    fd, socket.AF_INET, Echo())

I think.

[Socket]
ListenDatagram=9999

[Install]
WantedBy=sockets.target

I agree that we don't need a whole lot of documentation this, and that datagram endpoints should be there prefered solution.

(Regarding AdoptStreamEndpoint and adoptStreamPort, I think the lack of documentation is a defect).

As a note for future tickets, we are planning on moving to sentence-per-line, rather than 80-char limit, for documentation (see #6537)

This can be merged as-is, or you could add a mention of systemd or other uses cases, and then merge without further review.

comment:21 Changed 16 months ago by rwall

  • Status changed from new to assigned

I created #6612 for adding more socket adoption documentation.

comment:22 Changed 16 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [39027]) Merge udp-port-from-fd-5574-3: Add a twisted.internet.interfaces.IReactorSocket.adoptDatagramPort method

Author: rwall
Reviewers: tom.prince
Fixes: #5574

twisted.internet.interfaces.IReactorSocket now has a new
adoptDatagramPort method which is implemented by some reactors
allowing them to listen on UDP sockets set up by external software (eg
systemd or launchd).

comment:23 Changed 16 months ago by rwall

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [39069]) Revert r39027: Revert udp-port-from-fd-5574-3 because it introduced some unused imports.

http://buildbot.twistedmatrix.com/builders/pyflakes/builds/617/steps/pyflakes/logs/new%20pyflakes%20errors

twisted/internet/test/test_socket.py:17: 'fullyQualifiedName' imported but unused
twisted/internet/test/test_socket.py:24: 'SkipTest' imported but unused

Reopens: #5574

comment:24 Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted
  • Status changed from reopened to new

I removed the unused imports in r39070.

Build results now seem clean (apart from some bzr update failures):

comment:25 follow-up: Changed 16 months ago by therve

  • Keywords review removed
  • Owner set to rwall

As seen on the build, twistedchecker is still unhappy. You should at least try to fix the failures in non-test modules. Please merge once it improved. Thanks!

comment:26 Changed 16 months ago by rwall

  • Branch changed from branches/udp-port-from-fd-5574-3 to branches/udp-port-from-fd-5574-4

(In [39177]) Branching to 'udp-port-from-fd-5574-4'

comment:27 in reply to: ↑ 25 Changed 16 months ago by rwall

  • Status changed from new to assigned

Replying to therve:

As seen on the build, twistedchecker is still unhappy. You should at least try to fix the failures in non-test modules. Please merge once it improved. Thanks!

Thanks for the review.

  1. PosixReactorBase.adoptDatagramPort: This implementation follows the interface exactly, so I deliberately didn't document it so that pydoctor will insert the documentation from t.i.interfaces.IReactorSocket.adoptDatagramPort. I've got a twistedchecker branch awaiting review that fixes this. See https://bugs.launchpad.net/twistedchecker/+bug/1132540
  2. MemoryReactor.adoptDatagramPort: This does have a docstring that explains the specifics of this implementation and contains a link to the full interface documentation. This seems better than copying all interface documentation. It's also how the other MemoryReactor docstrings are written.
  3. Port._fromListeningDescriptor: Added missing parameter docstrings.
  4. twisted.internet.test.test_udp.ListenUDPMixin: On looking at these mixins again I don't think they're necessary. The getXXXerror methods were identical in each mixin and only called from one test so I just inlined the the code into the appropriate test. I moved the getListeningPort methods into the corresponding ReactorBuilder subclass. I documented the getListeningPort arguments where they differ from the wrapped adoptDatagramPort/listenUDP method.

If the builds still pass (with fewer twistedchecker warnings) I'll merge.

comment:28 Changed 16 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [39183]) Merge udp-port-from-fd-5574-4: Add a twisted.internet.interfaces.IReactorSocket.adoptDatagramPort method

Author: rwall
Reviewers: tom.prince, therve
Fixes: #5574

twisted.internet.interfaces.IReactorSocket now has a new
adoptDatagramPort method which is implemented by some reactors
allowing them to listen on UDP sockets set up by external software (eg
systemd or launchd).

Note: See TracTickets for help on using tickets.