Replace the implementation of IReactorSSL with one based on twisted.protocols.tls

[twisted-trac]

	@exarkun reported
Trac ID	trac#4854
Type	enhancement
Created	2011-02-07 22:23:43Z
Branch	https://github.com/twisted/twisted/tree/protocol-ssl-4854-6

The original implementation of IReactorSSL, shared amongst all reactors except for IOCP reactor, lets OpenSSL do all of the network operations, because that was the only way the pyOpenSSL bindings let them work.

More recently, pyOpenSSL began exposing an alternate OpenSSL API, which we now support in twisted.protocols.tls. This API lets us do all of the network operations and limits OpenSSL to just the crypto parts.

Finally, a benchmark shows that twisted.protocols.tls is actually comparable in performance to the IReactorSSL interface.

Since twisted.protocols.tls provides a struct superset of the functionality of IReactorSSL, we could implement the latter in terms of the former, removing some code in the process.

One possible issue is that pyOpenSSL 0.10 or newer is required for twisted.protocols.tls. This version of pyOpenSSL is a little over a year old, but may not yet be in widespread use. Possibly we should start by preferring a twisted.protocols.tls implementation of IReactorSSL, but falling back to the current implementation if necessary (for a while).

Searchable metadata

trac-id__4854 4854
type__enhancement enhancement
reporter__exarkun exarkun
priority__normal normal
milestone__ 
branch__branches_protocol_ssl_4854_6 branches/protocol-ssl-4854-6
branch_author__exarkun exarkun
status__closed closed
resolution__fixed fixed
component__core core
keywords__ 
time__1297117423000000 1297117423000000
changetime__1301852463000000 1301852463000000
version__None None
owner__exarkun exarkun
cc__thijs

[twisted-trac]

	@exarkun commented

Also, aside from the IReactorSSL code being somewhat complicated, there are outstanding bugs against it like #4455 which could be resolved by moving to a twisted.protocols.tls-based implementation.

[twisted-trac]

	Automation removed owner

[twisted-trac]

	@exarkun set owner to @exarkun @exarkun set status to assigned

[twisted-trac]

	@exarkun commented

(In [30792]) Branching to 'protocol-ssl-4854'

[twisted-trac]

	@itamarst commented

1. As part of this branch, we should check what versions of pyOpenSSL are shipped on distributions that package Twisted. If they're not keeping up with pyOpenSSL branches this may be an issue.

2. The new API hasn't been tested quite as much in the real world. Perhaps a better first step than replacing IReactorSSL with new implementation is simply reworking documentation and endpoints to use the new API. That plus deprecation warnings means users will switch to new APIs... but if it's broken for them they'll have the old implementation to fall back to.

[twisted-trac]

	@glyph commented

Replying to itamar:

1. As part of this branch, we should check what versions of pyOpenSSL are shipped on distributions that package Twisted. If they're not keeping up with pyOpenSSL branches this may be an issue.

"pyOpenSSL branches"? This is a feature of a pyOpenSSL release, not an experimental development thing. Version 0.10, which includes the requisite feature, was released over a year ago. Since then, 0.11 has come out.

Newer versions of software have newer versions of dependencies. Packagers will notice and upgrade as necessary; pyOpenSSL's compatibility policy is effectively as strict as Twisted's - perhaps even stricter, because it changes much less. In my opinion, it's the distributors' problem to contact us if they have an issue with a version upgrade.

For what it's worth though, Ubuntu Maverick packages 0.10, and so Ubuntu and its derivatives (and probably Debian too) area already caught up.

2. The new API hasn't been tested quite as much in the real world. Perhaps a better first step than replacing IReactorSSL with new implementation is simply reworking documentation and endpoints to use the new API. That plus deprecation warnings means users will switch to new APIs... but if it's broken for them they'll have the old implementation to fall back to.

If it's broken for them then maybe they should report bugs in it and we can fix them. We already have so many mechanisms in place to prevent regressions - unit test coverage, pre-release testing, buildbots on a dozen different platforms, a stable trunk that users can experimentally deploy on without too much worry - we should be fearless in releasing changes like this. If we need a whole slew of new tests in order to really be sure it's not broken and get better coverage, then that's fine (although I don't think we do), but there's no need to be cagey about releasing a feature that has passed our QA bar.

Those are the general arguments, but in this case we also have a specific argument. There are presumably people using iocpreactor with SSL (because there was a fair amount of interest in that ticket) and so this functionality has already been receiving real-world testing (in addition to the aforementioned unit test coverage and prerelease testing), and I haven't heard about significant problems with it yet.

Also, a major motivation for doing this ticket is to reduce the amount of code in Twisted and the complexity of the transport implementations. If we just switch over the endpoint implementations, we can't get rid of the code in the guts of the reactor and twisted.internet.tcp which facilitates this stuff.

Of course, maybe another point that got lost in the noise - should we just get rid of IReactorSSL entirely, and assume that the platform can provide an efficient in-memory SSL implementation, and deprecate it in favor of using the TLS endpoints? If we wanted to optimize things later on some different TLS implementation, we could always add magic hooks for twisted.protocols.tls to call if it's available.

[twisted-trac]

	@exarkun commented

and probably Debian too

Squeeze (stable) has 0.10, indeed.

I thought about making the implementation fall back to the old code if pyOpenSSL isn't new enough. This would be easy for connectSSL and listenSSL, but not very easy for startTLS. Since it's always been the case that we won't provide SSL APIs if pyOpenSSL isn't available, I think it's okay that going forward we might not provide SSL APIs if a new enough (>1 year old) version of pyOpenSSL isn't available.

Also, a major motivation for doing this ticket is to reduce the amount of code in Twisted and the complexity of the transport implementations.

Another motivation is that there's a really obscure bug in the current implementation that is most obviously fixed by replacing it with the twisted.protocols.tls-based implementation. :)

should we just get rid of IReactorSSL entirely

Ultimately, but I think this can wait a few more years. Everyone should be happily using endpoint APIs before we consider this.

we could always add magic hooks for twisted.protocols.tls to call if it's available

This might be a good use-case for non-str-based protocol interfaces. If we could move buffers (or other non-copying-based data structures) around, that should take care of the only (hypothetical) point of inefficiency in twisted.protocols.tls as compared to twisted.internet.ssl.

[twisted-trac]

	@itamarst commented

Latest checkin has incomplete sentence: "- a C{_tlsClientDefault} attribute indicating".

[twisted-trac]

	@exarkun commented

(In [30899]) Branching to 'protocol-ssl-4854-2'

[twisted-trac]

	@exarkun commented

(In [31214]) Branching to 'protocol-ssl-4854-3'

[twisted-trac]

	@exarkun removed owner @exarkun set status to new

build results

[twisted-trac]

	@exarkun commented

(In [31229]) Test and deprecate the Connection.TLS attribute

refs #4854

[twisted-trac]

	@glyph set owner to @glyph @glyph set status to assigned

[twisted-trac]

	@glyph set owner to @exarkun @glyph set status to new

1. _BypassTLS documentation should be a little clearer. First, its _base
   attribute is missing documentation. Also __getattr__ should have a
   docstring explaining that it relays every other attribute from the object that
   it wraps. (I say this because my first review point here was almost
   "_BypassTLS should relay other attributes, such as the producer attributes"
   but then I realized it already did.)
2. _startTLS could use some @types on its parameters. Especially
   bypass.
3. _tlsClientDefault needs an @ivar someplace.
4. So, the actual big deal in this branch is the OpenSSL version requirement
   bump. I'm not entirely sure if we should do that yet.
   1. First, there's the issue of how we should do it when we do it. (The code
   reduction in this branch is awesome, and I definitely want it to land, but
   it may be too early right now.) I have PyOpenSSL installed on this machine, but it's
   slightly too old of a version to work with this code. Yet there was no
   indication of why ssl.supported wasn't set until I ran the tests. I think
   that we need to emit some kind of loud warning that says "Hey, I see you've
   got PyOpenSSL, but it's too old a version for me to use". Also, the
   install documentation needs to list the new version requirement.
   2. Then there's the issue of whether it's premature to do this. I think
   that the key indicator that we shouldn't quite yet is that we removed SSL
   test coverage on some of our buildbots by accident, because their versions
   of PyOpenSSL were out of date.
   [http://buildbot.twistedmatrix.com/builders/fedora32-py2.5-reactors/builds/923/steps/poll/logs/stdio See the fedora32 build here], for example. (Our hardy builders are offline
   right now, but they are still in the supported list; they most likely would
   have died in a similar way.)
   3. But if we don't start moving forward, these old installations of
   PyOpenSSL will stick around forever, maybe. If, by some miracle, this were
   to get in by tomorrow, the way I'd like to see it land is this: keep the old
   grotty legacy code, unmodified, but move it aside into a superclass which is
   only conditionally included (or something like that). Then, emit a warning
   if it gets used, and by default use the new twisted.protocols.tls path
   when it's possible. I don't think we should necessarily have to do this
   with every single optional dependency version bump, but this one is pretty
   critical.
5. I don't see a NEWS file; please add one.

[twisted-trac]

	@jyknight commented

This only requires a newer version of pyOpenSSL, not of OpenSSL, right? If someone's gonna upgrade Twisted on an old distro, I don't see much issue also upgrading pyOpenSSL to go with it.

[twisted-trac]

	@exarkun commented

(In [31269]) Branching to 'protocol-ssl-4854-4'

[twisted-trac]

	@exarkun commented

I don't see much issue also upgrading pyOpenSSL to go with it

I tend to agree. But I didn't see your comment until after I mostly implemented the fallback for older versions.

[twisted-trac]

	@exarkun removed owner

Latest build results

[twisted-trac]

	@glyph set owner to @exarkun

• twisted.internet._newtls and twisted.internet._oldtls module docstrings should be more specific about exactly what versions of pyOpenSSL will cause them to be used. INSTALL should probably say something about 0.10 always being recommended, but being required on Windows.
• New mixin objects in twisted.internet._newtls should be new-style.
• _newtls._startTLS probably doesn't need to be extra-double-private. One underscore is enough since it's not on an object that gets re-exported somewhere else, it's just a function. Feel free to add the underscore when it's imported into a public namespace if you want to be extra paranoid about it. For that matter, don't bother changing this if you don't feel like it; I just thought I should mention it.
• File a separate ticket to emit some kind of warning when the old implementation is used. Figuring out the timing of that warning is probably tricky, so let's not block this getting merged on it.
• The asserts in posixbase should be real exceptions.
• There's one suspicious failure on the win7 buildbot here: http://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/329/steps/select/logs/problems

[FAIL]
Traceback (most recent call last):
  File "c:\Users\exarkun\twistedbot\windows7-64-py2.7-select\Twisted\twisted\test\test_tcp.py", line 1289, in clientDisconnected
    self.assertEqual(err.args[0], expectedErrorCode)
twisted.trial.unittest.FailTest: not equal:
a = [('SSL routines', 'SSL_write', 'protocol is shutdown')]
b = 10038


twisted.test.test_ssl.StolenTCPTestCase.test_properlyCloseFiles

I forced a rebuild here - http://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/344 - and it failed again in exactly the same way. Curiously, it fails on the select reactor, but not on the IOCP reactor.

It's too bad about the Windows thing, because everything else was pretty cosmetic. If you can figure that one out, then it should probably be landed, although a test failure with unknown functional changes to fix necessitates a re-review. Please link to the revision from your re-review comment though, as that will (hopefully) be easy to review on its own.

[twisted-trac]

	@exarkun commented

(In [31366]) Change the special case error handling to account for any reactor using twisted.protocols.tls

refs #4854

[twisted-trac]

	@exarkun commented

(In [31367]) Be explicit about pyOpenSSL version dependency for these modules

refs #4854

[twisted-trac]

	@exarkun commented

(In [31369]) Make the newtls mixins new-style

refs #4854

[twisted-trac]

	@exarkun commented

(In [31370]) Single private startTLS helper

refs #4854

[twisted-trac]

	@exarkun removed owner

twisted.internet._newtls and twisted.internet._oldtls module docstrings should be more specific about exactly what versions of pyOpenSSL will cause them to be used. INSTALL should probably say something about 0.10 always being recommended, but being required on Windows

Done in r31367 and r31372. Followup fix in r31373 to fix the markup.

New mixin objects in twisted.internet._newtls should be new-style.

Done in r31369

_newtls._startTLS probably doesn't need to be extra-double-private.

Done in r31370

File a separate ticket to emit some kind of warning when the old implementation is used.

Filed #4974

The asserts in posixbase should be real exceptions

I agree. They're asserts in trunk, though. I filed #4975 for addressing them.

There's one suspicious failure on the win7 buildbot here

Fortunately, it was nothing horrible at all. The failing test, twisted.test.test_ssl.StolenTCPTestCase.test_properlyCloseFiles made an assertion about the exception passed to a IProtocol.connectionLost implementation. However, the exception could already vary based on a couple factors:

• Windows vs any other platform
• twisted.protocols.tls-based IReactorSSL implementation (ie iocp) vs not

On Windows with twisted.protocols.tls and on all other platforms the error expected would be one about illegally writing to a shutdown SSL connection. However, on Windows without twisted.protocols.tls, a platform quirk slipped through and the test would see WSAENOTSOCK, due to whatever random OpenSSL internals got involved. So the test worked around this inconsistency by expecting WSAENOTSOCK in certain cases.

Now that the reactor prefers twisted.protocols.tls, some cases where WSAENOTSOCK might come up have been eliminated. So in r31366 I changed the test to expect it in only the remaining cases - namely, on Windows and with pyOpenSSL <0.10.

Here are the latest build results.

[twisted-trac]

	@glyph commented

The patch looks good, but... while testing this with calendar server, I saw this at the bottom of a bunch of tracebacks when I stress-tested it by making tons of SSL connections. (I haven't done any other testing yet, not sure if I can reproduce this with just Twisted itself.)

  File "twisted/internet/tcp.py", line 184, in writeSequence
    self.protocol.writeSequence(iovec)
exceptions.AttributeError: 'Server' object has no attribute 'protocol'

Any idea what that might be about?

[twisted-trac]

	@exarkun set owner to @glyph

Any idea what that might be about?

Not really. I couldn't reproduce it with the calendarserver test suite or with CalDAVTester. What did you do?

[twisted-trac]

	@glyph commented

Replying to exarkun:

Any idea what that might be about?

Not really. I couldn't reproduce it with the calendarserver test suite or with CalDAVTester. What did you do?

Refresh my calendars a few times in succession, or, type in an https:// URL in a browser and whack ⌘R really fast.

I think it may have something to do with connections being dropped by the client before the server thinks the negotiation has finished...?

[twisted-trac]

	@exarkun set owner to @exarkun @exarkun set status to assigned

This is a problem when a protocol writes to a disconnected server-side transport SSL transport. The old implementation silently discarded such writes.

[twisted-trac]

	@exarkun commented

(In [31396]) Add a not-quite-working test for the misbehavior (breaking many other things in the process)

refs #4854

[twisted-trac]

	@exarkun commented

(In [31398]) Branching to 'protocol-ssl-4854-5'

[twisted-trac]

	@exarkun commented

I think this is nearly ready for another review. It's just waiting for #4987 to be merged so we can deal with the gtk2 issues on Windows.

[twisted-trac]

	@exarkun commented

(In [31476]) Back out changes which depend on [#4854](#4854)

refs #4987
refs #4854
refs #3371

[twisted-trac]

	@exarkun commented

(In [31478]) Branching to 'protocol-ssl-4854-6'

[twisted-trac]

	@exarkun removed owner @exarkun set status to new

Okay, one more time!

1. twisted.protocols.tls is preferred by all reactors to implement IReactorSSL and ITLSTransport if it is available (which is determined by whether pyOpenSSL 0.10 or newer is available)
2. As before, IOCP reactor can only use twisted.protocols.tls to implement these interfaces
3. A test in test_ssl.py is changed to expect the failure mode you get from twisted.protocols.tls instead of the old failure mode if twisted.protocols.tls is in use.
4. twisted.internet.ssl has some minor import-related cleanups (not strictly related to this change, sorry)
5. Some new tests for writing to closed connections have been added (asserting the previous, not-very-nice behavior of silently swallowing the write)
6. twisted.internet.test.reactormixins gets a minor tweak, moving Glib2Reactor and Gtk2Reactor to POSIX-only, as they always should have been, since PortableGtkReactor is what you actually get on Windows if you want to use Gtk. It was nice to pretend that the former two reactors could work on Windows, but that fantasy is stretched past the breaking point by the new write-after-disconnect tests added in this branch (ie, they fail the new tests badly, and not because of the changes in this branch).

Latest build results

[twisted-trac]

	@exarkun commented

Oh yes. Also, I did not fix the CalendarServer problem. CalendarServer is poking around in reactor internals. I think this implementation simplification (weeeeell it'll be a simplification eventually, when we drop support for pyOpenSSL <0.10) is more important than supporting extremely obscure uses of internal reactor APIs. It should be an easy change to CalendarServer to avoid the problem, and the problem is very minor since it is only logging an error in a case where data is being discarded, as opposed to silently discarding that data as it used to do.

[twisted-trac]

	@thijstriemstra set owner to @exarkun

Some doc stuff:

twisted/internet/_newtls:

• could use an @since marker
• startTLS's transport param docstring's 1st paragraph should end with 'has' instead of 'have'

twisted/internet/_oldtls:

• could use an @since marker
• none of the classes have docstrings

twisted/internet/_ssl:

• could use an @since marker

twisted/protocols/tls:

• It has the following sentence:

Or it can be used to run TLS over a UNIX socket, or over stdio to a child process.

What about It can also be used to run TLS over a UNIX socket, or over stdio to a child process. That stdio part still flows odd though, not sure what else to suggest..

INSTALL:

• The last sentence has On which should be lowercase.

twisted/internet/tcp:

• docstring for BaseClient should be on 3 lines

twisted/topfiles/4854.bugfix:

• there's no mention of pyopenssl or the support for both older/new versions of pyopenssl

[twisted-trac]

	@exarkun commented

(In [31482]) address review feedback

refs #4854

[twisted-trac]

	@exarkun removed owner

could use an @SInCE marker

Hm. It's private. I think @SInCE markers are mostly for public APIs so developers know what minimum version of Twisted they will need if they use them. On the other hand, it doesn't hurt (for the module level, at least).

startTLS's transport param docstring's 1st paragraph should end with 'has' instead of 'have'

I think it's correct as it is. "have" agrees with the plural subject, "requirements" (though putting a verb before a ":" is traditionally considered bad form, but I never really agreed with that :).

none of the classes have docstrings

I added docstrings for the truly new classes (and made them new-style). I left the old classes, which merely moved to a new module, undocumented. Documenting them understandably would be quite a challenge, plus what they do almost doesn't make sense anyway (shuffling __class__s around and so forth), and I hope that we can delete them within another release or two.

That stdio part still flows odd though, not sure what else to suggest..

I changed it in another way, hopefully that reads better.

Last three points fixed as well.

Thanks for the review!

[twisted-trac]

	@glyph commented

Replying to exarkun:

Oh yes. Also, I did not fix the CalendarServer problem. CalendarServer is poking around in reactor internals. I think this implementation simplification (weeeeell it'll be a simplification eventually, when we drop support for pyOpenSSL <0.10) is more important than supporting extremely obscure uses of internal reactor APIs. It should be an easy change to CalendarServer to avoid the problem, and the problem is very minor since it is only logging an error in a case where data is being discarded, as opposed to silently discarding that data as it used to do.

What do you mean by "the Calendar Server problem"? There's the warnings and there's the traceback; I wouldn't expect you to fix the warnings, but I think the traceback is worth addressing.

I think it's slightly misleading to call this an "extremely obscure use of internal reactor APIs". The use-case here is to distribute load between different processes on the same system to make use of multiple cores. That's a pretty common desire, I think. There's no nice, neat public API way to do it.

We should definitely fix Calendar Server to do it better (although I'm still not completely clear on how to do that...) but we also need to have documentation that explains how it should be done, before the only semi-obvious way to do it starts emitting tracebacks.

Of course you could contribute a fix to Calendar Server to address the problem or absorb the code from Calendar Server into Twisted somewhere and my objections would go away ;).

Basically, I agree that these are reactor internals, but we should go through the process of fully deprecating all of twisted.internet.ssl, and for that matter, twisted.internet.tcp, while at the same time adding properly exposed versions of the functionality there so I can get my file descriptors and sockets from places other than a direct call to socket.socket().

[twisted-trac]

	@exarkun commented

Okay. I fixed things so Calendar Server's usage works. Apple should get motivated about implementing some APIs so this can be done in a more reasonable manner, though. ;)

Latest build results

[twisted-trac]

	@glyph set owner to @exarkun

This looks pretty good; I looked at the Calendar Server issue under both PyOpenSSL 0.10
and 0.7. I notice I didn't see any warnings either; thanks for that :). I
really think the implementation is ready to go, but there are just a few
maintenance documentation issues that we should tie off before landing.

1. There are a lot of implied bits of work here that have been deferred for
a later time or for separate ticket, so we should make sure all that gets
tracked; this code is a minefield of implicit and entirely non-obvious
facts about SSL and OpenSSL versions.
    1. `_oldtls` should have a link in it to [[#4974](https://github.com/twisted/twisted/issues/4974)](https://github.com/twisted/twisted/issues/4974).
    2. We don't need to up our ticket count for this right now, but #4974
    should at least indicate that there should be a separate ticket later
    for removing `_oldtls`.
    3. The docstring for `_oldtls` should say at least a little something
    about ''why'' this implementation is bad.  Or it should just reference
    `_newtls`, which explains why this implementation is good.
    4. I think the ultimate direction that we talked about for TLS support
    was to move away from `connectSSL` and `Transport.startTLS` entirely,
    and do something like [#3204](https://github.com/twisted/twisted/issues/3204) for switching to TLS.  In particular I
    believe we've talked about making the SSL endpoints be implemented
    without calling `listenSSL` or `connectSSL`.  I think this should be
    recorded in a ticket.
    5. The `ConnectionMixin.TLS` attribute seems like it should go away at
    some point.  Separate ticket for deprecation, I guess.
2. A couple of things are not documented.  I'm intentionally ignoring
everything in `_oldtls`, because screw that noise.
    1. `twisted.internet._newtls.ConnectionMixin` says it's "a mixin for 'a
    transport'".  But it's a bit more specific than that.  Something has a
    `dataBuffer` attribute which it requires, and it should point a bit more
    directly at that.  Given that it calls classmethods of `FileDescriptor`,
    I assume it should refer directly to that :).
        1. `loseConnection` and `doWrite` aren't documented on
        `ConnectionMixin`.  These don't strike me as too important.
    2. `twisted.internet._newtls.ClientMixin`
    3. `twisted.internet._newtls.ServerMixin` - these two should probably
    mention the implementation mechanism in `twisted.internet.tcp`.
    4. `twisted.internet.test.test_tcp.FakeSocket`; the added methods and
    attributes.
        1. `send`
        2. `shutdown`
        3. `close`
        4. `fileno` (and there should be 3 blanks after the class, not 2)
        5. It seems like `close` and `shutdown` should have a flag which
        affects the behavior of `send`, causing it to raise an exception.  I
        think this might catch some implementation issues in the future.
    5. `twisted.internet.test.test_tcp._FakeFDSetReactor`
        1. Also it should implement `IFDSetReactor`
    6. `twisted.internet.test.connectionmixins.serverFactoryFor` - this
    docstring isn't too specific. (I had to read the implementation to
    figure out what it meant), and this function is simple enough that it
    could be somewhere public.  File a ticket for this and add a link.  (Or,
    if you can find one, just add a link to the existing one.)
    7. `twisted.internet.tcp.Server._base`: I know that you're not
    introducing this attribute, but you are moving it, and it is... subtle.
    It'd be great if you could add a note about this.
3. I'm glad there's no warning initially, but [#4974](https://github.com/twisted/twisted/issues/4974) should have a timeline
associated with it.  I'm assuming that it should be done in 11.2?  (Maybe...
put it in a milestone?)  Anyway, I'm okay with any timeline you suggest, it
should just be clear what you intend.

The implementation looks good though. The only actually functional suggestion
here was the close thing, which is purely in the tests, and shouldn't actually
affect these tests at all. Address these issues to your satisfaction and land.

[twisted-trac]

	@exarkun commented

   1. There are a lot of implied bits of work here that have been deferred for a later time or for separate ticket, so we should make sure all that gets tracked; this code is a minefield of implicit and entirely non-obvious facts about SSL and OpenSSL versions.
         1. _oldtls should have a link in it to #4974.

Done, http://twistedmatrix.com/trac/changeset/31536#file1

         2. We don't need to up our ticket count for this right now, but #4974 should at least indicate that there should be a separate ticket later for removing _oldtls.

If not now, then it'll just be later. :) I filed #5014.

         3. The docstring for _oldtls should say at least a little something about why this implementation is bad. Or it should just reference _newtls, which explains why this implementation is good.

Done, change also visible at http://twistedmatrix.com/trac/changeset/31536#file1

         4. I think the ultimate direction that we talked about for TLS support was to move away from connectSSL and Transport.startTLS entirely, and do something like #3204 for switching to TLS. In particular I believe we've talked about making the SSL endpoints be implemented without calling listenSSL or connectSSL. I think this should be recorded in a ticket.

Hmmm. Yes. Filed #5015.

         5. The ConnectionMixin.TLS attribute seems like it should go away at some point. Separate ticket for deprecation, I guess. 

I suppose. I don't care so much about this either way though. It will go away when we resolve #5015, after all.

   2. A couple of things are not documented. I'm intentionally ignoring everything in _oldtls, because screw that noise.
         1. twisted.internet._newtls.ConnectionMixin says it's "a mixin for 'a transport'". But it's a bit more specific than that. Something has a dataBuffer attribute which it requires, and it should point a bit more directly at that. Given that it calls classmethods of FileDescriptor, I assume it should refer directly to that :).
               1. loseConnection and doWrite aren't documented on ConnectionMixin. These don't strike me as too important. 
         2. twisted.internet._newtls.ClientMixin
         3. twisted.internet._newtls.ServerMixin - these two should probably mention the implementation mechanism in twisted.internet.tcp.

Done, see http://twistedmatrix.com/trac/changeset/31536#file0 (ClientMixin and ServerMixin had docstrings already, but not describing their purpose, I assume this is what you were asking for)

         4. twisted.internet.test.test_tcp.FakeSocket; the added methods and attributes.
               1. send
               2. shutdown
               3. close
               4. fileno (and there should be 3 blanks after the class, not 2)

Done, see http://twistedmatrix.com/trac/changeset/31536#file4

               5. It seems like close and shutdown should have a flag which affects the behavior of send, causing it to raise an exception. I think this might catch some implementation issues in the future. 

Hmm. Yea maybe... But this class shouldn't pretend to be a general purpose socket fake. It does what is necessary for the few tests that use it now. I guess we should have a ticket for making it a general purpose fake, but I'm hesitant for some reason...

         5. twisted.internet.test.test_tcp._FakeFDSetReactor
               1. Also it should implement IFDSetReactor 

Done, change is visible at the bottom of http://twistedmatrix.com/trac/changeset/31536#file4

         6. twisted.internet.test.connectionmixins.serverFactoryFor - this docstring isn't too specific. (I had to read the implementation to figure out what it meant), and this function is simple enough that it could be somewhere public. File a ticket for this and add a link. (Or, if you can find one, just add a link to the existing one.)

Filed #5016.

         7. twisted.internet.tcp.Server._base: I know that you're not introducing this attribute, but you are moving it, and it is... subtle. It'd be great if you could add a note about this. 

Done, see http://twistedmatrix.com/trac/changeset/31536#file2

   3. I'm glad there's no warning initially, but #4974 should have a timeline associated with it. I'm assuming that it should be done in 11.2? (Maybe... put it in a milestone?) Anyway, I'm okay with any timeline you suggest, it should just be clear what you intend. 

#5014 proposes a timeline now. I think having it in a milestone is a good idea, too.

[twisted-trac]

	@exarkun set status to closed

(In [31537]) Merge protocol-ssl-4854-6

Author: exarkun
Reviewer: itamar, glyph, thijs
Fixes: #4854
Refs: #4974
Refs: #5014
Refs: #4455

Add an implementation of IReactorSSL and ITLSTransport which uses the memory
BIO APIs present in pyOpenSSL 0.10 and newer. This implementation will be preferred
by all reactors if the pyOpenSSL dependency is satisfied, otherwise the old
implementation will still be used.

This appears to have slightly better performance than the old implementation and
should avoid bugs like #4455.