On May 19, 2014, at 11:24 AM, [email protected] wrote:
> Prior to #7098, these were things that implemented a `getContext` method that
> returned an `OpenSSL.SSL.Context` instance.
I should note that this was poorly documented everywhere (in fact, one of the
main things I noticed when working on the cluster of problems related to #7098
was that nothing really explained what a "context factory" was).
> Subsequent to #7098, these are now *either* that or an object that provides
> `IOpenSSLClientConnectionCreator`.
Yes, this is correct.
> However, other parts of Twisted itself were not updated. For example, the
> layers that sit in between `twisted.protocols.tls` and
> `twisted.web.client.Agent` weren't touched much.
Are you referring to anything other than SSL4ClientEndpoint?
> `SSL4ClientEndpoint`, for example, still documents its `sslContextFactory` as
> "SSL Configuration information as an instance of
> L{twisted.internet.ssl.ContextFactory}.". And, somewhat insanely I think,
> `IReactorSSL.connectSSL` still says "@param contextFactory: a
> L{twisted.internet.ssl.ClientContextFactory} object.".
"Insanely" might be overstating it. Incorrectly, maybe :-).
> Merely from a documentation standpoint, this seems suboptimal. From a
> compatibility standpoint... Well, it seems incompatible to me. Perhaps this
> is an instance where the compatibility policy can be broken (though really
> that's academic since 14.0.0 has already been released, the policy has been
> broken already) but I don't recall any explicit discussion about a decision
> to do this.
At the time, I thought really hard about how to perform this change in a
compatible way, and I thought I'd come up with something that was in line with
the compatibility policy. Apparently my reviewer agreed. I even thought I
discussed it with you, since you were sitting right there ;-). Perhaps it was
someone else at the sprint. Upon reflection though, I think you're right, and
it is technically incompatible. I only say "technically" because I don't think
third-party implementations exist, not because this type of incompatibility
should be OK or the policy should be changed.
My reasoning went like this:
If you can import the new interface to declare that you implement it, then of
course your contextFactory expects to have the new methods declared on it. So
there's no possibility of passing in an object which does not provide that
interface and getting a surprise AttributeError.
If you want to work with older versions of Twisted and span the compatibility
gap, then it's easy enough to determine if the new interface is available.
To be clear, the case I didn't consider, the thing that makes this an
incompatible change, is that if you have a new version of Twisted, where the
new interfaces are available, and your shiny new
IOpenSSLClientConnectionCreator provides them, but does not provide the
old-style "getContext", then a perfectly valid 3rd-party implementation of
IReactorSSL will unconditionally call getContext on your fancy new object and
explode.
In other words, IReactorSSL has changed incompatibly, because the type of one
of its arguments has changed incompatibly.
If I had done this only to, for example, SSL4ClientEndpoint's constructor, I
think it actually would have been a compatible change. In order to provoke
this behavior in that case, you'd need to monkeypatch SSL4ClientEndpoint itself.
> I *hope* and suspect there won't be much fall-out from this change
> considering it's hard to implement TLS and as far as I know there are no
> third-party implementations of `IReactorSSL` (GNUTLS came to mind but they
> have their own incompatible interface afaict). In other words, maybe we'll
> get lucky this time.
I did check various code-search sites to see how folks were using it, and...
yes, basically nobody has implemented IReactorSSL, as far as I can tell. I did
learn that a lot of people vendor in Twisted though: do you know Twisted 10.2
apparently is in Chromium's build tools directory? Anyway, if someone had
actually implemented it, this problem might have occurred to me earlier.
If we were going to make this mistake and learn from it, then this seems like
the ideal place to have done so.
> I wrote this email instead of filing tickets about the documentation problems
> because doing the latter was implicit acknowledgement that this incompatible
> change is okay. Having written the email now, I see there's probably no
> going back, regardless. Maybe we can learn something from this incident and
> avoid repeating it with a more popular interface, though.
So, just from a technical perspective, how could we have avoided this?
If we were to go with the suggested 'version' attribute on interfaces, I think
that we would still have basically the same problem. We could increment
IReactorSSL to version 2, but that still obliges the caller with the new-style
context factory to always check that attribute before attempting to call it.
That seems like a pyrrhic victory; satisfying our currently stated victory
condition without actually satisfying the goal of not breaking software that
implements IReactorSSL. In fairness, it does, at least, give the developer of
the offending code a way to fix it, but the developer of the offending code
(the one that calls IReactorSSL) is unlikely to be the one who notices it.
We could have declared a new interface (Or IReactorSSL version 2) with a new
method, connectSSL_Ex, which had the new signature. In that case, at least,
you'd be aware that you were calling a new method that might not be available
on older Twisted versions and might think to check the presence of
IReactorSSL_2 or IReactorSSL.version.
I'd say we could have only changed SSL4ClientEndpoint/SSL4ServerEndpoint and
just planned to deprecate IReactorSSL entirely, but that would have left
'ITLSTransport' in the lurch, using crappy old interfaces, unless we
implemented generalized protocol switching.
This still leaves me scratching my head as to how I could have noticed the
change was incompatible in time, though, which is possibly the more interesting
question. I guess I do read CompatibilityPolicy from time to time so this
suggestion might have worked:
> We could probably add a description of this particular kind of incompatible
> change to the `CompatibilityPolicy` wiki page. If reviewers read that page,
> then they'll know to watch out for it.
"Don't change the type of any public interface's arguments; for example: ..."?
I was going to say something about how we might want certain interfaces to not
allow 3rd-party implementations, except there are plenty of 3rd-party places
that IReactorSSL is imported... they just happen to inherit our existing
implementation of the interface, because it's the one everyone wants anyway.
> And of course (assuming we're committed to this direction, which we seem to
> be) we need to fix the rest of the "contextFactory" documentation throughout
> Twisted. I'll go file one ticket related to that now...
Yes, this seems like some necessary follow-up work; reverting at this point
would be pretty pointless.
-g_______________________________________________
Twisted-Python mailing list
[email protected]
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
