Opened 7 years ago

Last modified 3 years ago

#4471 enhancement new

Add IDatagramEndpoint interface and implementations

Reported by: glyph Owned by:
Priority: normal Milestone:
Component: core Keywords: udp endpoints
Cc: rwall Branch: branches/datagram-endpoints-4471
branch-diff, diff-cov, branch-cov, buildbot
Author: jonathanj


As described on #1442, there should be datagram endpoint interfaces to parallel the stream endpoint interfaces.

Attachments (1)

idatagram-endpoint-4471.patch (8.9 KB) - added by rwall 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by glyph

(In [29147]) Merge endpoints-1442-5: a high level connection and listening API

Author: rwall, dreid, glyph

Reviewer: radix, exarkun, glyph, jknight

Fixes: #1442

Refs: #4470 Refs: #4471 Refs: #4472 Refs: #4473 Refs: #3204

Added new "endpoint" interfaces in twisted.internet.interfaces, which abstractly describe stream transport endpoints which can be listened on or connected to. Implementations for TCP and SSL clients and servers are present in twisted.internet.endpoints. Notably, client endpoints' connect() methods return cancellable Deferreds, so code written to use them can bypass the awkward "ClientFactory.clientConnectionFailed" and "Connector.stopConnecting" methods, and handle errbacks from or cancel the returned deferred, respectively.

comment:2 follow-up: Changed 4 years ago by jerub

  • Owner changed from glyph to jerub

I have been working on this ticket.

comment:3 in reply to: ↑ 2 Changed 4 years ago by rwall

  • Cc rwall added

Replying to jerub:

I have been working on this ticket.

Hi Jerub,

I'd quite like to help out with this. Is there any code? I couldn't find a branch.

Ultimately I'd like to be able to launch twisted DNS using systemd socket activation. This seems like the first step.

See also:

Changed 3 years ago by rwall

comment:4 Changed 3 years ago by rwall

See attachment:idatagram-endpoint-4471.patch

  1. I wanted to see if / where automatic socket type detection (#5599) could be used.
  2. So I copied existing Stream interfaces and classes to give some rough Datagram equivalents.
  3. There are no tests and the docstrings are all wrong.
  4. Not a very useful patch, but it might prompt some discussion of what needs doing for IDatagramEndpoint.

comment:5 Changed 3 years ago by rwall

  • Keywords UDP review added
  • Owner jerub deleted

This ticket has been difficult to find if you search for UDP rather than Datagram.

Also jonathanj suggested that the attached patch might be worth putting up for review.

The patch shows what I had to do to get a DatagramEndpoint for testing stuff in #5599.

Please comment on the proposed IDatagramEndpoint API and the associated implementations and whether the other stuff should moved to separate tickets.

comment:6 Changed 3 years ago by jonathanj

  • Author set to jonathanj
  • Branch set to branches/datagram-endpoints-4471

(In [39997]) Branching to 'datagram-endpoints-4471'

comment:7 Changed 3 years ago by jonathanj

  • Keywords udp endpoints added; UDP removed

I opted for only implementing UDP server endpoints in this branch, since that seems like a small enough piece of work to be useful without introducing too much complexity (for me and for any reviewers.) There is also no equivalent of twisted.application.strports.service for datagram server endpoints, at the moment.

comment:9 Changed 3 years ago by lvh

  • Owner set to lvh

comment:10 Changed 3 years ago by lvh

  • Keywords review removed
  • Owner lvh deleted

AAAARGH, I just lost a bunch of review work on this because FF ate the comment box.

That said, thank you for your contribution :) Mostly nitpicks, but the meat and potatoes of this patch looks really good.

Boring stuff:

  • Missing full stops
    • twisted/internet/ L523, L541, L1304
  • Epytext markup issues
    • twisted/internet/ L524, L527, L1309, L1312
    • twisted/internet/ L2513, add @type protocol: L{twisted.internet.interfaces.IProtocol}
    • twisted/internet/ L2557: link to prefix directly. Also, isn't in an attribute instead of a method?
    • datagramServerFromString: add types for reactor and description params? See also comment below about str type
    • Wrap "description" with C{} here
  • In parseDatagramServer, twisted/internet/ L1322, is the lack of a docstring which appears to be replaced by two full-line comments intentional? Are you trying to not clobber an interface docstring perhaps?
  • For Python3 compatibility, we should be explicit about the kind of str type we mean. Currently this says str, I think bytestrings are meant, so it should be L{bytes}.


cheers lvh

Note: See TracTickets for help on using tickets.