Opened 4 years ago

Last modified 11 months 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
(diff, github, buildbot, log)
Author: jonathanj Launchpad Bug:

Description

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 15 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 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 18 months ago by jerub

  • Owner changed from glyph to jerub

I have been working on this ticket.

comment:3 in reply to: ↑ 2 Changed 17 months 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 15 months ago by rwall

comment:4 Changed 15 months 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 12 months 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 12 months ago by jonathanj

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

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

comment:7 Changed 12 months 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 11 months ago by lvh

  • Owner set to lvh

comment:10 Changed 11 months 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/endpoints.py: L523, L541, L1304
  • Epytext markup issues
    • twisted/internet/endpoints.py: L524, L527, L1309, L1312
    • twisted/internet/interfaces.py: L2513, add @type protocol: L{twisted.internet.interfaces.IProtocol}
    • twisted/internet/interfaces.py 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/endpoints.py 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}.

Nitpicks:

cheers
lvh

Note: See TracTickets for help on using tickets.