Opened 7 years ago

Last modified 4 years ago

#4471 enhancement new

Add IDatagramEndpoint interface and implementations

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

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 Richard Wall 4 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 Changed 4 years ago by Stephen Thorne

Owner: changed from Glyph to Stephen Thorne

I have been working on this ticket.

comment:3 in reply to:  2 Changed 4 years ago by Richard Wall

Cc: Richard Wall 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 4 years ago by Richard Wall

comment:4 Changed 4 years ago by Richard Wall

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 4 years ago by Richard Wall

Keywords: UDP review added
Owner: Stephen Thorne 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 4 years ago by Jonathan Jacobs

Author: jonathanj
Branch: branches/datagram-endpoints-4471

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

comment:7 Changed 4 years ago by Jonathan Jacobs

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:8 Changed 4 years ago by Jonathan Jacobs

comment:9 Changed 4 years ago by lvh

Owner: set to lvh

comment:10 Changed 4 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/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.