Opened 7 years ago

Last modified 3 months ago

#3204 enhancement new

provide a clean access mechanism to access and switch protocols from an ITransport provider

Reported by: glyph Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: davidsarah, adi@…, daniele.athome@… Branch: branches/switch-protocols-3204
(diff, github, buildbot, log)
Author: itamar Launchpad Bug:

Description

Some protocols - such as any protocol with a "STARTTLS" command - allow you to switch to a different application-level protocol in the middle of a connection.

Currently, Twisted contains two kinds of hacks to make this happen. One, as in TLS support, gets into the guts of the reactor implementation to allow you to switch protocols. Another, as in AMP's protocol switch command, adds an additional layer of (inefficient, overcomplex) wrapping in order to connect a different protocol to the available transport.

Currently, tcp.Connection happens to have a protocol attribute, but this is a mostly undocumented accident; it would be good to have an explicit API to switch protocols associated with ITransport.

I don't have a specific use-case at the moment for accessing the protocol attribute, but it seems like it would be nice to provide access along with mutation.

Change History (13)

comment:1 follow-up: Changed 7 years ago by jml

The ticket's subject sounds not quite right. In the case of TLS support, aren't you switching both the transport (from TCP to TLS) and the protocol (from something that allow STARTTLS to something that doesn't)?

The changes to the transport interface seem obvious:

class ITransport(Interface):
    def getProtocol():
        """Return the current protocol."""

    def setProtocol(protocol):
        """Change the current protocol."""

We could quite easily add both of these, have getProtocol return something useful and have setProtocol raise NotImplementedError. I think the YAGNI principle rejects this though.

However, I don't think it's such a bad thing if sometimes you can't setProtocol, because either a protocol or a transport forbids it.

AIUI, the open questions are:

  • How would we implement setProtocol?
  • (related) Are there any changes that need to happen to the protocol interface to make this happen?

comment:2 in reply to: ↑ 1 Changed 7 years ago by glyph

Replying to jml:

The ticket's subject sounds not quite right. In the case of TLS support, aren't you switching both the transport (from TCP to TLS) and the protocol (from something that allow STARTTLS to something that doesn't)?

What you're doing now, in Twisted terms, is switching the transport from TCP to TLS. However, this is a broken metaphor; TLS is a protocol that goes over TCP. It's also a transport. So what you're really doing is switching the protocol for the TCP transport to TLS(originalProtocol).

The changes to the transport interface seem obvious:

Please let's never have another method whose name begins with 'get'.

class ITransport(Interface):
    def getProtocol():
        """Return the current protocol."""

    def setProtocol(protocol):
        """Change the current protocol."""

We could quite easily add both of these, have getProtocol return something useful and have setProtocol raise NotImplementedError. I think the YAGNI principle rejects this though.

I don't understand the motivation for having it raise an error rather than actually work.

However, I don't think it's such a bad thing if sometimes you can't setProtocol, because either a protocol or a transport forbids it.

Why? There's no particular reason a protocol or transport should be able to forbid it, that I can think of. The only thing it would do is stop delivering data to the existing protocol and start delivering to the new one.

AIUI, the open questions are:

  • How would we implement setProtocol?
self.protocol = newProtocol
newProtocol.makeConnection(self)
# and, maybe
self.protocol.dataReceived(remainingData)
  • (related) Are there any changes that need to happen to the protocol interface to make this happen?

I don't think so...

comment:3 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:4 Changed 4 years ago by <automation>

  • Owner glyph deleted

comment:5 Changed 4 years ago by exarkun

See #5015 for the TLS use case of this API.

comment:6 Changed 4 years ago by davidsarah

  • Cc davidsarah added

See #5028 for a related problem due to foolscap setting the protocol attribute directly (which no longer works in twisted trunk).

comment:7 Changed 3 years ago by itamar

It is possible that other people have implemented ITransport providers, which we don't want to make invalid. More significantly, we have many providers in twisted, and testing all of them with new interface in a single ticket will be a lot of work.

I therefore suggest adding sub-interface ISwitchableTransport with getProtocol() and setProtocol(p, extraBytes), and as a first pass just switching a minimal set (TCP, TLS) and opening tickets for other providers (loopback, conch, stdio, ...). Most of the use cases we've encountered have been in TCP and TLS.

comment:8 Changed 3 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/switch-protocols-3204

(In [33568]) Branching to 'switch-protocols-3204'

comment:9 Changed 3 years ago by exarkun

It is possible that other people have implemented ITransport providers, which we don't want to make invalid.

Oops, abortConnection.

comment:10 Changed 3 years ago by itamar

Current status: it ought to work for both TCP and TLS, has basic tests. I do, however, want to make some higher level tests that involve connected transports, data delivery and connectionLost notification, which will have to wait for #5392 to be merged.

comment:11 Changed 3 years ago by itamar

  • Author changed from itamarst to itamar
  • Owner set to itamar

comment:12 Changed 2 years ago by adiroiban

  • Cc adi@… added

comment:13 Changed 3 months ago by daniele_athome

  • Cc daniele.athome@… added
Note: See TracTickets for help on using tickets.