Endpoints, a flexible high level connection API.

[dreid]

A high-level connection API which eliminates the need for semantically confusing and unecessary ClientFactory. Also provides the advantage of "remembering" connection information for creating more connections to the same endpoint. The interfaces provided a symetrical between Client and Server, providing a single method that takes a callable which is called for protocol creation.

See also twisted/words/protocols/jabber/jstrports.py for an example use case.

[dreid]

A high-level connection API which eliminates the need for semantically confusing
and unecessary ClientFactory.  Also provides the advantage of "remembering"
connection information for creating more connections to the same endpoint.  
The interfaces provided a symetrical between Client and Server, providing a
single method that takes a callable which is called for protocol creation.

branch: dreid/endpoints

[rwall]

A link to dreid's work on this - log:branches/dreid/endpoints#15734

[rwall]

Dreid's branch as a diff against current trunk

[rwall]

A simple example of endpoints usage.

[rwall]

exarkun suggested simply adding connect and listen methods to IAddress. I've created a new ticket #2060 for this.

[rwall]

Dreid's branch, with regular factories, SSL support, and some tests.

[rwall]

TODO:

•  Combine connect and listen into a single endpoint class
• Use something simpler than policies.ProtocolWrapper.

[rwall]

This time with connect and listen united, a simpler wrapping protocol, and documented IAddress.buildEndpoint

[dreid]

The fixme previously in WrappingProtocol was refering to the possibility of just pointing self.transport at the new protocol and then removing itself from the process entirely. I'm not sure there is any supported way to do that though.

And with regards to UDP I'm fairly sure I remember glyph telling me that they should be a special interface anyway IUDPEndpoint but i'm not sure I believed him at the time. Other than that this is looking pretty good. I'll resurrect the branch and commit the latest patch so it'll be easier when it comes time to review.

branched to source:branches/endpoints-1442

[rwall]

I noticed that at the moment, the logs are prefixed with _WrapperFactory, _WrapperProtocol. This is an experiment to wrap just the connectionMade method rather than entire protocol. With this, the logs are prefixed correctly once the connection is made. Not sure if it's a good idea though.

[rwall]

It seems to me that if the IProtocolFactory and IProtocol interfaces included something like addEventListener(phase, phase, eventType, callable, *args, **kwargs), then _WrapperFactory and _WrapperProtocol wouldn't be necessary. eg

d = defer.Deferred()
def cb(proto):
    """Do stuff with the protocol"""
    pass
d.addCallback(cb)

def onProtocolBuilt(proto, d):
    #proto = return value of IProtocolFactory.buildProtocol
    def onProtoConnectionMade(res, proto, d):
        #res = The result of IProtocol.connectionMade
        d.callback(proto)
    proto.addEventListener("after", "connectionMade", onProtoConnectionMade, proto, d)
factory.addEventListener("after", "buildProtocol", onProtocolBuilt, d)

[rwall]

Some tests and fixes for the address module.

[rwall]

Exarkun, you mentioned on IRC that you'd be willing to have a look at this, so I'm assigning it to you for review.

[radix]

Ok, so I've reviewed this, and asked exarkun about some things as well. Here we go.

Clear-cut things, in order of difficulty (easy to hard :)

• Please go through all your new and modified modules, classes, and methods, and add docstrings to all of them, including the test methods (clarifying their intent).

• IServerEndpoint is not consistent with itself: The method doesn't take a reactor argument, but the docstring says it does.

• IClientEndpoint and IServerEndpoint probably should not be concerned with the reactor at all: The implementation should take a reactor in its initializer.

• _WrappingProtocol and _WrappingFactory, if the strategy they use is actually correct, should take advantage of the wrapping helpers in twisted.protocols.policies. Ideally these wouldn't be necessary at all, but we can probably fix that once the transport API grows some more useful methods.

• SSL stuff should not be stuck into the TCPEndpoint. Please separate it out into a different class (and taking into consideration my next point)

• I don't think Client and Server endpoints should be mixed into the same classes. There may be a use case for it, but the current implementations are really muddy and the use cases for having endpoints that can either be connected to or listened on are both 1. not obviously correct and 2. not urgent. In the future, we can easily add an object which mixes in (or delegates to) both a TCPClientEndpoint and TCPServerEndpoint, so we should put it off. As for IAddress.buildEndpoint, I suggest leaving it out for this branch, and perhaps add it in a later branch once we figure out exactly what we want to do about it. Do you already have a compelling use case for it?

Discussion items:

• It's unclear that connect() should just return a Deferred: One use-case that connectTCP facilitates that this doesn't is connector.disconnect(). If endpoints are used, there's no clear way to stop a connection attempt before it is made.

Thanks for working on this! It's getting closer.

[rwall]

A diff against endpoints-1442 addressing most of radix's review points.

[rwall]

Replying to radix:

Ok, so I've reviewed this, and asked exarkun about some things as well. Here we go. Clear-cut things, in order of difficulty (easy to hard :) * Please go through all your new and modified modules, classes, and methods, and add docstrings to all of them, including the test methods (clarifying their intent).

Done. Except where docstrings are already present in an Interface definition.

* IServerEndpoint is not consistent with itself: The method doesn't take a reactor argument, but the docstring says it does.

Done.

* IClientEndpoint and IServerEndpoint probably should not be concerned with the reactor at all: The implementation should take a reactor in its initializer.

Done.

* _WrappingProtocol and _WrappingFactory, if the strategy they use is actually correct, should take advantage of the wrapping helpers in twisted.protocols.policies. Ideally these wouldn't be necessary at all, but we can probably fix that once the transport API grows some more useful methods.

As mentioned in #twisted, twisted.protocols.policies seemed to cause a circular import problem.

* SSL stuff should not be stuck into the TCPEndpoint. Please separate it out into a different class (and taking into consideration my next point)

Done.

* I don't think Client and Server endpoints should be mixed into the same classes. There may be a use case for it, but the current implementations are really muddy and the use cases for having endpoints that can either be connected to or listened on are both 1. not obviously correct and 2. not urgent. In the future, we can easily add an object which mixes in (or delegates to) both a TCPClientEndpoint and TCPServerEndpoint, so we should put it off.

Done.

As for IAddress.buildEndpoint, I suggest leaving it out for this branch, and perhaps add it in a later branch once we figure out exactly what we want to do about it. Do you already have a compelling use case for it?

Removed. It didn't make much sense for SSL, UDP anyway.

Discussion items: * It's unclear that connect() should just return a Deferred: One use-case that connectTCP facilitates that this doesn't is connector.disconnect(). If endpoints are used, there's no clear way to stop a connection attempt before it is made.

I don't have a usecase for returning a deferred, but I think dreid may have.

Thanks for working on this! It's getting closer.

No problem. I no longer have a use for endpoints though and I was getting pretty lost with what you were all discussing last night, so I'm not sure I'm the best person to carry on this work.

Anyway have a look at endpoints4.diff above.

[exarkun]

James, can you attach the notes you took regarding this?

There's some coding left to do, dropping the review tag.

[jknight]

There was a discussion about endpoints, and everybody was mostly positive about it. Here were some things I wrote down from the discussion, I dunno if they make much sense though. ;)

• Endpoints should be addresses
• Probably want to remove the need for factories eventually.
  • have to add notifyOnDisconnect to transport for that
  • but don't do that yet! Accept factories for now, add the ability to accept Protocols to the same API later.
• No endpoints until after tw 2.5
• connectStream/connectDatagram returns Deferred.
• cancellable Deferreds to cancel connecting.

[dreid]

Ok, I was going to write some code tonight but as I was sitting down to do it I first looked at the endpoints-1442 branch (which seems to have all the latest patches from rwall in it. So I decided to review his code instead, and it is so almost amazingly close to being correct for clients.

So a couple of points on where we are and what needs to be done to finish up endpoints.

• Endpoints are not addresses and should not be addresses - Endpoints are where all that ephemeral information about a connection goes that isn't it's address.
• IServerEndpoint.listen shouldn't result in a protocol, it should result in a Port at which point it probably shouldn't even bother returning a deferred (or atleast it shouldn't need to be more than defer.execute() around the reactor.listen* calls.) Unless you wanted to make it not result in the Port until we were actually listening (which I could see maybe having some usecases.) but I don't think there is an easy way to do this.
• It would be really nice if IClientEndpoint.connect returned 1) a cancellable deferred, 2) something that you could call .disconnect on after you were done with the client. But for #2 proto.transport.loseConnection() is probably good enough.
• I'm not entirely convinced that the symmetry of IServerEndpoint is entirely necessary to have an awesome and useful endpoints implementation. So I don't think it would be the worst thing in the world if it was left out of the initial merge. I personally just don't see a compelling usecase for them.

[glyph]

See #739 for a compelling use-case. strports, for example, should be able to parse some string junk into a server endpoint so that the parsed value can be passed around rather than passing the string junk itself around and expecting everyone to know what to do with it. When you parse a "port", you really want to parse a "thing that can listen", not an integer.

[dreid]

merged forward to source:branches/endpoints-1442-2

This branch only implements Client and Server endpoints for TCP, SSL, and UNIX. And I think it addresses all the issues brought up in review so far. Plus I added some WrappingFactory and WrappingProtocol unittests.

[glyph]

It looks like you haven't dealt with your own review comments yet, and those are the ones that immediately occur to me :). Especially the elimination of _WrappingProtocol.

To clarify the design: getHost and getPeer should eventually return IEndpoint providers, but I would like to see that as a discrete step 2, since there are separate issues there.

IEndpoint.listen should return a Deferred which fires a Port. IEndpoint.connect should return a Deferred which fires a Connector when it starts trying to connect, not when it has connected. We don't need it to return a protocol, because it'll be possible to nicely and generically implement something like ClientCreator around these.

The reactor-backed implementation will do as you suggest, and simply defer.execute(), but there are several use-cases which might involve deferred listen/connect:

• Vertex needs to ask another process to do the listen or connect on its behalf.
• SOCKS does basically the same thing, I think (but I'm not familiar enough with its semantics to be sure).
• It would be nice to be able to listenSSL or connectSSL with a key with a passphrase and not block on getting the passphrase, and instead request it asynchronously from e.g. a GTK dialog box. That would not actually start connecting or listening until later.

All of these are further implementation work, not part of this ticket, but since they're a large part of my original motivation for wanting this, I think they should be accounted for in the interface.

I'm not entirely against splitting client and server endpoints into separate branches (smallness is good), but then we should have 2 new tickets.

Speaking of splitting though, I think the address changes are probably okay, but should be in a separate ticket.

More minor stuff:

• address changes:
  • __eq__ and __str__ are being changed, they should get docstrings.
  • setUpClass is problematic within trial for various reasons, and should really only be used as an optimization; those should really just be setUp, since they're not expensive.
  • the new test module should have a docstring.
• endpoints:
  • twisted.internet.endpoints needs to have a docstring.
  • I think "object representing" is not terribly useful redundancy in the docstrings. If you're going to be redundant, maybe say something a bit clearer like "A client endpoint is a place that ClientFactory can connect to. For example, a remote TCP host/port pair would be a TCP client endpoint. For other examples, see twisted.internet.interfaces.".

[exarkun]

It seems quite wrong to me to have IEndpoint.connect return a Deferred which fires with a Connector. Connectors are basically valueless, and ClientCreator can only be implemented based on this API because ClientCreator is implemented wrong.

I think IEndpoint.connect needs to return a Deferred which fires with a Protocol.

[jknight]

+1, what I've been saying all along. This is the opportunity to kill the stupid extra classes in the API.

However, look at what that implies:

From an above message (when connect *did* return a Deferred, before (!)):

It's unclear that connect() should just return a Deferred: One use-case that connectTCP facilitates that this doesn't is connector.disconnect(). If endpoints are used, there's no clear way to stop a connection attempt before it is made.

Well, guess how I'd propose that that be implemented...

[exarkun]

+1 to Deferred cancellation, but we can worry about that later on.

[jknight]

Do you suggest implementing the first draft of this without connection cancellation? Seems acceptable to me.

[exarkun]

yea, that

[glyph]

I agree that connector objects are effectively valueless, and endpoint objects should return Deferreds that can be canceled. However, I disagree that we can worry about it later on.

If we don't make deferred cancellation a requirement of this API, there is a risk that people writing new applications will avoid using endpoints because folk knowledge will develop that they are less featureful than connectTCP et. al.. Such folk knowledge almost always far outlives the actual feature or bug that it concerns.

I might be swayed if this were a big job or a gnarly dependency, but I don't see any particular reason to worry about it later, either. We even had deferred cancellation for a little while, and as per #1236, it seems the only reason it was removed was the lack of a motivating use-case. David could simply reverse r14693 into this branch and then use it, or in the interest of smaller branches, review/merge that quickly in anticipation of this motivating use-case.

However, this concern only affects client endpoints. It sounds like we have consensus on server endpoints. If there really is a reason to defer cancelability, we can still merge that first.

[exarkun]

It doesn't bother me if Deferred cancellation is added first, either. It should not be added in this branch though.

From a general software development standpoint, though, adding another dependency to this ticket seems unlikely to bring it to completion more quickly.

[glyph]

Rather than creating a procedural holdup here, I've put #990 into review. Hopefully we can merge that ticket before this discussion is even completely over ;).

[exarkun]

Adding a reference to jstrports to the description (and copying the initial roundup comment into the description)

[dreid]

(In [28379]) Branching to 'endpoints-1442-3'

[dreid]

(In [28431]) Branching to 'endpoints-1442-4'

[dreid]

endpoints-1442-4 appears to address all the still relevant review points from previous reviews and supports deferred cancellation.

[therve]

Some comments:

    Protocol whose only function is to callback deferreds on my
    factory when I am connected and when I am disconnected.

We should not use "I" in the docstring anymore. It's done in a couple of places.

        Test that an exception raised in buildProtocol of our wrappedFactory
        results in our onConnection errback being fired.

You can remove "Test that". It's done in multiple places. Also, code should be wrapped with C{} or L{}

        (ep, expectedArgs, expectedDest) = self.createClientEndpoint(
            mreactor, clientFactory)

The parenthesis are superfluous.

There are a bunch of missing blank lines.

        Test that the callable returned by
        L{endpoints.connecterCancellorFactory} stops connecting the
        L{IConnector} and errback's it's deferred.

 http://www.angryflower.com/itsits.gif

failUnless and assertEqual should be replace by assertEquals.

            wf._canceller.connector = connector
            return wf._onConnection

It would be nice to have a public method to do those both things.

[dreid]

Ok, I addressed the doc string issues, I also added a few more doc strings to some of the complicated test harness code.

_WrappingFactory should only ever be used by the endpoint implementations so it's fine for both of these to be private attributes (exarkun agreed.)

[therve]

There are a couple of uncovered code in endpoints:

    1:     def __init__(self, deferred):
               """
               @param deferred: The L{Deferred} that could not be cancelled
               """
>>>>>>         Exception.__init__(
>>>>>>             self,
>>>>>>             "Canceller.canceller called without a connector: %r" % (deferred))
>>>>>>         self.deferred = deferred

    4:         if self.connector is None:
>>>>>>             raise NoConnectorError(deferred)


    1:     def connectionLost(self, reason):
               """
               Proxy C{connectionLost} calls to our C{self._wrappedProtocol}
               """
>>>>>>         return self._wrappedProtocol.connectionLost(reason)

    1:     def clientConnectionFailed(self, connector, reason):
               """
               Errback the C{self._onConnection} L{Deferred} when the
               client connection fails.
               """
>>>>>>         self._onConnection.errback(reason)

[glyph]

I think it would be good to update the description of this ticket, since the "semantically confusing and unnecessary ClientFactory" will not in fact be removed by this change. Parts of it are indeed redundant: clientConnectionLost is redundant with connectionLost on your protocol object, and clientConnectionFailed is redundant with the Deferred returned by connect being errbacked, However, there's still no clean way to address buildProtocol being able to refuse connections without instantiating the IProtocol provider in the first place. buildProtocol receives the address object before the protocol is instantiated, and the particular protocol being instantiated may be dependent upon the contents of that address.

[jknight]

"eliminates the need for" != "eliminates". And it does eliminate the need for.

[jknight]

I'm not sure about the "4" suffix on the various components. I think it actually makes sense to use the same APIs for IPv4 and IPv6 in Twisted. That needs to be determined before merging this in with "4" names.

[dreid]

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/endpoints-1442-4

[glyph]

Replying to jknight:

"eliminates the need for" != "eliminates". And it does eliminate the need for.

Unfortunately, it does not, at least as currently implemented. IClientStreamEndpoint.connect takes an IClientFactory as a parameter...

but now that I'm thinking about it, you're right. It only actually needs an IProtocolFactory, because the only thing that isn't redundant with the Deferred's notifications is buildProtocol, which isn't an IClientFactory-specific thing.

I think that this means that we should actually make that argument be an IProtocolFactory instead, and emit a deprecation warning when an IClientFactory is used. I feel like I could be convinced pretty easily if there's some facet of that that I haven't considered, though.

[glyph]

Replying to jknight:

I'm not sure about the "4" suffix on the various components. I think it actually makes sense to use the same APIs for IPv4 and IPv6 in Twisted. That needs to be determined before merging this in with "4" names.

I think that it's important to be able to select whether you want (A) TCP4 only, (B) TCP6 only, or (C) some combination of the two. I'm sure that Pavel, for example, will want all of his communications to go over SCTP/IPv6.

This branch should have the versions with '4' in the names, because we'll need those either way; I do think that we should follow it up with a TCP6 and DWIM endpoints. Actually, my hope for the real high-level API that we end up with will be something like twisted.internet.strpoints.listener('something:...').listen(...) and twisted.internet.strpoints.connector('something-else:...').connect(...), where 'something:' and 'somethingElse:' are plugin namespaces where external libraries can plug in their own transport layer endpoints - but I digress, and I wanted to avoid dealing with that digression on ''this ticket.

So I think we should keep the '4', but eventually we might want to point to another API in the docstrings of these classes.

[jknight]

Replying to glyph:

So I think we should keep the '4', but eventually we might want to point to another API in the docstrings of these classes.

Disagree, see comment my last comment on #3014. I think instead connectTCP/etc should have an optional address family argument, which lets you decide to only use ipv4 or ipv6, and endpoints should follow.

[glyph]

Replying to jknight:

Disagree, see comment my last comment on #3014. I think instead connectTCP/etc should have an optional address family argument, which lets you decide to only use ipv4 or ipv6, and endpoints should follow.

Reasonable as the comment that you refer to is - and I'm currently inclined to agree - I think that there's more discussion yet to happen in the area of IPv6. Think of this as the reference implementation of the API. The important thing here, in my opinion, is to get code to use I*Endpoint interfaces.

Think of the "4" version as a reference implementation. I really, really don't want to block this ticket on agreement on the IPv6 code. So, if we do agree on your suggestions for IPv6 implementation, we can deprecate that class, and that's fine, because at most that is going to require one character of code to update its users: -TCP4...Endpoint +TCP...Endpoint. Whereas if we decide to go a different way, then "TCP...Endpoint" is going to have the wrong behavior and we're going to have to think of some wonky new name and deprecate something that sounds obvious.

But, what people should really be doing is using a pluggable API for 99% of applications, so the important thing is to get the pluggable API in place so we don't have to argue about it, and users can decide based on administrative configuration whether they want IPv6 or not, rather than code. Stalling for consensus on IPv6 will slow down that process, even if it means we get IPv6 right, it means that most users will have to wait longer before their code can start supporting it (because if you use the explicitly pluggable API your code is advertising that it can support some level of changing behavior behind the declared interface).

(The only thing that I can think of that shouldn't be able to work with an arbitrary stream endpoint is Vertex, and that's only because it needs to know the difference between different stream types so that it can publish information about network topology.)

[glyph]

1. As several of the previous comments indicate, we have a bunch of things that we're going to want to do in the future. We've had to say "we're not going to block this" on a bunch of stuff, but to really see the benefits of endpoints, we're going to need some of those things. So before I get into the review part of the review, I'm going to enumerate all of those tickets, since they should at least exist before this is closed. Somebody needs to file these, but I highly recommend you delegate as much as possible.
   1. As James Knight noted, there should be stream endpoints capable of IPv6 TCP.
   2. There should be an IDatagram(X)Endpoint to parallel IStream(X)Endpoint.
   3. As Jean-Paul Calderone mentioned on IRC, To facilitate legacy migrations, there should be a wrapper around IStreamClientEndpoint which can take a ClientFactory instance. The rationale here is that there's too much code in Twisted right now that requires either clientConnectionFailed or clientConnectionLost, and we need to make adopting endpoints as easy as humanly possible, so that protocol code can stop binding so tightly to the reactor. You shouldn't have to depend on getting rid of ClientFactory to get the new awesomeness of endpoints.
      1. This new class shouldn't be deprecated per se, but its documentation should clearly indicate that it is for legacy convenience.
      2. passing a ClientFactory to IStreamClientEndpoint.connect is formally correct; it implements the required interface. However, it is somewhat misleading and the code should probably emit a warning pointing at this legacy convenience class.
   4. My personal crusade: there should be an addition to (or replacement for) strports which:
      1. can produce providers of every one of the interfaces:
         1. IStreamClientEndpoint, IDatagramClientEndpoint, IDatagramClientEndpoint, IDatagramServerEndpoint
      2. has a plug-in interface so that new types of endpoints can be registered.
      3. accepts uniform user input so that users can enter different endpoints regardless of interface. practically speaking, this mean "its input is a string, and users should be able to type that string", but there are lots of ways that it could be specified so that other forms of input would also be acceptable.
2. There are a few issues with the code & documentation:
   1. listenArgs and connectArgs are weird. Why keyword args but not positional args? Why an argument with a default that actually takes a dict rather than varargs? My suggestion would be to literally duplicate all the arguments to IReactorTCP.connectTCP and IReactorSSL.connectSSL. This is tedious, but nice and explicit, and provides good documentation (you can stay on the pydoctor page for the endpoint interface, and not have to go look at the reactor).
   2. _ConnectorCanceller seems redundant. Can you just delete it and move its method and attribute to _WrappingFactory? If this is actually important for some reason (avoiding circular references, maybe? I haven't thought it all the way through.) then please just document it a bit.
   3. Documentation of public stuff shouldn't refer to private stuff. NoConnectorError ought to say what the exception means, not what code will internally raise it.
      1. There's also no such thing as "Canceller.canceller", so NoConnectorError's error message shouldn't describe it.
      2. At the very least, NoConnectorError should say what the public API is that raises it.
   4. New exceptions should go into twisted.internet.error, not twisted.internet.endpoints. This is both for the sake of consistency and also because catching an exception should not require necessarily loading the code which raises that exception.
   5. As James Knight noted, the clientFactory parameter would be better named protocolFactory, to avoid confusion. Similarly I think the serverFactory parameter would be better named protocolFactory as well. For users approaching this code, "server" and "client" are somewhat ambiguous terms which may refer to multiple levels of the stack, but within the context of Twisted, 'protocol' means something fairly specific.
3. minor nits:
   1. twisted.internet.endpoints starts with "# -*- test-case-name: twisted.test.test_endpoints -*-", but I'm pretty sure you meant twisted.internet.test.test_endpoints.
   2. endpoints.py's copyright goes back a bit too far, doesn't it?
   3. The twisted.internet.endpoints module docstring should really L{point} at the endpoints interfaces, rather than just use the generic term "endpoints".
   4. Update copyright on twisted/internet/address.py

Thanks for your sustained and substantial efforts on this code. I think it's very close.

[glyph]

1. ConnectingCancelledError says that it is raised when an IConnector is cancelled, but I think you mean an IStreamClientEndpoint. IConnector never raises that exception.
2. _WrappingProtocol needs to deal with IHalfCloseableProtocol so that the reactor's providedBy detection will properly relay half-close notifications. It should only provide it, probably using directlyProvides, if its wrapped protocol does. See tcp.py around line 489.

That's all I can find. Since that second point will probably require some more tests this will likely need one more review, but that should be it!

[jknight]

Add to the list of related tickets to file:

Adding extra functionality in the reactor/wherever to allow the simplification of the endpoints implementation. Specifically, removal of _WrappingProtocol. This level of hack shouldn't be necessary for something that is part of Twisted, when the appropriate modifications can be done to make it unnecessary instead.

When all the tickets are filed, make sure to leave a comment here pointing to them.

[glyph]

Replying to jknight:

Add to the list of related tickets to file: Adding extra functionality in the reactor/wherever to allow the simplification of the endpoints implementation. Specifically, removal of _WrappingProtocol. This level of hack shouldn't be necessary for something that is part of Twisted, when the appropriate modifications can be done to make it unnecessary instead. When all the tickets are filed, make sure to leave a comment here pointing to them.

This ticket already exists, and AMP needs it too: http://twistedmatrix.com/trac/ticket/3204

[glyph]

One review point left:

1. Write a NEWS entry.

Then, get somebody to file those tickets, and edit the description here to point at them all.

You can go ahead and merge it first, but you're not off the hook until that's done! :).

Something else I noticed, although this was a precedent before this branch so you're clearly not on the hook to fix it, is that the verifyObject calls in proto_helpers really ought to be in a test, not just sitting at the module level, so you can get some useful feedback if the interfaces change.

[jknight]

This ticket already exists, and AMP needs it too: http://twistedmatrix.com/trac/ticket/3204

I think that's a different thing. Endpoints currently is written in terms of the existing public API of twisted. This means that in order to fire the "connection made" Deferred at the appropriate time, it has to hook into Protocol.connectionMade. That'd be fine if endpoints was just a 3rd party package.

However, as its goal is to actually *become the new* recommended public API of twisted, it makes more sense to make internal API changes as required to make its implementation more straightforward and thus more easily maintainable with the rest of Twisted. It doesn't actually make sense long-term for the main "make a connection" API in twisted to have to make up a fake protocol just to make the deferred get fired at the right point.

[glyph]

Replying to jknight:

This ticket already exists, and AMP needs it too: http://twistedmatrix.com/trac/ticket/3204

I think that's a different thing. Endpoints currently is written in terms of the existing public API of twisted. This means that in order to fire the "connection made" Deferred at the appropriate time, it has to hook into Protocol.connectionMade. That'd be fine if endpoints was just a 3rd party package. However, as its goal is to actually *become the new* recommended public API of twisted, it makes more sense to make internal API changes as required to make its implementation more straightforward and thus more easily maintainable with the rest of Twisted. It doesn't actually make sense long-term for the main "make a connection" API in twisted to have to make up a fake protocol just to make the deferred get fired at the right point.

I'm not sure. In every API that emits a Deferred, there's a lower-level callback that gets invoked. The interaction between buildProtocol and connectionMade is maybe a bit undesirable, but it makes sense to me, especially as you may need to do this anyway to implement endpoints which involve a reversed connection. (calling connect() but eventually getting a server socket, as you might with a NAT traversal library).

Keep in mind that although endpoints are designed to be the high-level API for connecting to other hosts, there's still the issue of the low-level API, which you need to be aware of if you are going to implement a reactor plugin, for example. Endpoints should still be able to hook into third-party reactors sanely. So it's not the only "public" API, even if it is the one we would encourage users to examine first.

However, if you want to file such a ticket, go for it. It would definitely be nice if the internals could be cleaned up a little.

[exarkun]

I'm a bit uncertain about the general utility of the proto_helpers.py changes. These are clearly useful for the endpoints tests, but what about other tests?

At the very least, I wonder why FakePort gets to be in proto_helpers.py while StubConnector lives in test_endpoints.py, and why the ports are created with an IPv4 address of "localhost", which isn't even a valid value for that address field.

My particular motivation for looking here is that I'm trying to write tests that verify a connector is disconnected and thought the changes in this branch might be useful. They may be a start, but they don't quite go far enough, and half of the necessary functionality is in the wrong place.

[glyph]

Replying to exarkun:

I'm a bit uncertain about the general utility of the proto_helpers.py changes. These are clearly useful for the endpoints tests, but what about other tests?

Could this refactoring be done in another ticket? I mean, proto_helpers is private, right? (Ha ha ha.) So we shouldn't have to worry about exposing more public names there?

[exarkun]

Yea.. not really. proto_helpers is in use by other projects. Calling it private at this point probably isn't the most productive course of action.

[exarkun]

I stole some of the more generally useful proto_helpers.py changes for #4329. There'll be a conflict with this branch, but maybe I'll merge this forward and resolve it tomorrow.

[exarkun]

I closed #2060 as a duplicate of this.

[glyph]

(In [29086]) Branching to 'endpoints-1442-5'

[glyph]

(In [29120]) Correct description of StubConnector (it doesn't actually even have a "disconnecting" attribute) refs #1442

[glyph]

(In [29137]) As requested by exarkun, expose fewer public names in proto_helpers, and more consistently implement IConnector and IListeningPort. Also, add test cases for new MemoryReactor features, since it's really test infrastructure and not test code. Finally, simplify endpoints tests and remove somewhat spurious dependence on task.Clock.

refs #1442

[glyph]

Tickets described above have been filed as #4470, #4471, #4472, and #4473.

[glyph]

Just to be safe, one final  pre-flight build.

[glyph]

Aaeeugh. Well, py-without-modules saves the day. Removing hard dependency on PyOpenSSL.

[glyph]

(In [29146]) Skip SSL tests when SSL is unavailable (and don't import a private name when there's a perfectly good public alias). refs #1442

[glyph]

OK. Another round of build results yields only known errors at the point this branch diverged from trunk; some (but sadly, not all) of these have now been fixed in trunk. Importantly, py-without-modules is now green. Preparing to land...

[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.