XmlStreamServerFactory arbitrarily holds on to the result of authenticatorFactory

[glyph]

In r24961, XmlStreamServerFactory was introduced.

Rather than simply passing the argument that it wants to pass to its XmlStream, it stuffs the result of authenticatorFactory() into an instance attribute. This causes the lifecycle of its authenticator to be surprising; rather than getting collected when the connection does, it has to wait around for the next connection to be made.

This also causes it to destructively modify the public, documented "args" attribute of XmlStreamFactoryMixin, without saying that it will do so, violating its parent's documented behavior. Users can now pass keyword arguments but not positional arguments by setting these attributes, which seems kind of random.

My suggested resolution would be to remove the usage of XmlStreamFactoryMixin entirely, since it seems to providing less than zero utility in this case.

Technically this isn't a regression, since it's new functionality, so I'm not going to revert right away: but I'd definitely appreciate it if this could be fixed before it's enshrined in a release.

[thijs]

also add an @since: 8.2 tag to XmlStreamServerFactory when you fix it, the committer forgot to add this.

[ralphm]

As I mentioned in #3435, the solution to use the args attribute isn't pretty and I didn't think of this side effect. However this factory needs to depend on XmlStreamFactoryMixin because of the bootstrap observer functionality.

I only documented the args and kwargs attributes in the same commit, by the way.

One solution would be to duplicate the buildProtocol code to set up the observers, but that isn't particularly nice, either.

Suggestions welcome.

[glyph]

I suggest factoring up the observer setup logic into a base class that doesn't do the apparently unnecessary and undesirable args/kwargs stuff. SimpleXMLStreamFactory, if you will.

It's unfortunate that this involves a proliferation of new public names, but hey, that's better than a proliferation of subtly broken semantics.

By the way, a coding standard nit that I didn't notice until now: the coding standard specifically says "Acronyms should be capitalized in their entirety." That means "XMLFoo", "HTTPFoo", not "XmlFoo" and "HttpFoo". You could take advantage of this to mitigate the number of new public names. For example, you could deprecate XmlStreamFactoryMixin and make a new XMLStreamFactoryMixin which only does the stuff that you apparently actually need (i.e. the observer setup).

[exarkun]

Before anyone takes the name suggestions SimpleXMLStreamFactory seriously, let me point out that you shouldn't. No API in Twisted should include the word simple to differentiate it from some other similar API.

[ralphm]

(In [24965]) Branching to 'jabber-xmlstreamfactorymixin-3464'

[ralphm]

(In [24966]) Split out test cases for bootstrap observers.

This makes the test_buildProtocol also use self.factory, which nicely highlights the destructive nature of t.w.p.j.xmlstream.XmlStreamServerFactory. Re #3464.

[ralphm]

I created a new mixin BootstrapMixin that implements the bootstrap code previously in XmlStreamFactoryMixin and let XmlStreamServerFactory derive directly from it, while implementing its own buildProtocol. This looks far nicer than the previous solution.

I also added the @since: 8.2, as requested. I wasn't aware that this needed to be done.

I suppose XmlStreamFactoryMixin could be deprecated now, except that t.w.x.xmlstream.XmlStreamFactory derives from it to inherit the args/kwargs stuff. In turn, t.w.p.j.xmlstream.XmlStreamFactory derives from the latter. However, it overrides the __init__ arguments by requiring just an authenticator instance. As far as I know, nobody really uses the args/kwargs stuff directly, and I would be fine with it being removed. Suggestions welcome, but this should probably go into another ticket.

Please review.

[ralphm]

Oh, I forgot about this: while I was already aware of the naming conventions w.r.t. acronyms, there are a lot of places in the Jabber implementation in Twisted that use Xml instead of XML. This has been in Twisted for years now. I don't think it is a very good idea to rename all that or have some use the one and some the other. Also note that in newer code, other acronyms, like SASL, TLS and XMPP are all used in uppercase.

[exarkun]

1. Wouldn't test_addAndRemoveBootstrap be better as a blackbox test? You could call dispatch like in test_installBootstraps and assert that the removed callback isn't called.
2. BootstrapMixin.addBootstrap and removeBootstrap don't document their parameters or exceptions.
3. the line self.installBootstraps(xs) in twisted/words/protocols/jabber/xmlstream.py in XmlStreamServerFactory.buildProtocol is untested
4. It'd probably be nice if there were a test for the authenticator factory passed to XmlStreamServerFactory.__init__ actually being used - as it is, the factory could just unconditionally use twisted.words.protocols.jabber.xmlstream.Authenticator

[ralphm]

(In [25129]) Address review comments.

Re #3464.

[ralphm]

Replying to exarkun:

1. Wouldn't test_addAndRemoveBootstrap be better as a blackbox test? You could call dispatch like in test_installBootstraps and assert that the removed callback isn't called.

That'd be better indeed. Fixed.

2. BootstrapMixin.addBootstrap and removeBootstrap don't document their parameters or exceptions.

I already mentioned the parameters in the class docstring, but added them to the methods, too, now.

3. the line self.installBootstraps(xs) in twisted/words/protocols/jabber/xmlstream.py in XmlStreamServerFactory.buildProtocol is untested

Oh, I missed that one. Earlier versions inherited that test from the mixin test case. Fixed.

4. It'd probably be nice if there were a test for the authenticator factory passed to XmlStreamServerFactory.__init__ actually being used - as it is, the factory could just unconditionally use twisted.words.protocols.jabber.xmlstream.Authenticator

I create a dummy authenticator and test against that now.

Please review.

[exarkun]

XmlStreamServerFactoryTest.test_buildProtocol seems to be testing several distinct behaviors:

1. that XmlStreamServerFactory invokes the callable passed to its __init__ and saves the result as its authenticator attribute
2. that the created authenticator has its associateWithStream method called
3. that the bootstrap observers registered on the factory are then registered on the XmlStream returned by buildProtocol

These are all great things to test, but I think there should be three test methods instead of one. At the very least, it is totally necessary for test_buildProtocol's docstring to enumerate each of these points. Without that, a future maintainer might easily be led to believe that two thirds of this test method is unnecessary cruft which can be deleted. It seems other tests indirectly rely on two out of three of these behaviors, so the maintainer will notice if they break it, but if they only change the test to simplify it (not any implementation and therefore don't break anything), they may not realize they are deleting the only direct test coverage for some behavior. Debugging the other tests which would then fail if the functionality gets broken will be unpleasant. It's easy to split up the test into three parts, so there's probably no reason not to.

Also, several tests use a Deferred's callback method as an event listener, but none of these tests is really asynchronous. The failure mode for the tests which use a Deferred like this is that they time out after two minutes. If the tests all used (eg) a list's append method and asserted about the contents (or at least the length, which would be equivalent to what the tests do now) then the failures would be immediate.

[ralphm]

I split that test in four different tests of which two have been collected in a testing mixin class that is also now used by XmlStreamFactoryMixinTest. Also removed the use of Deferred in the test cases touched by this branch.

Please review.

[exarkun]

Looking good.

1. in twisted/words/test/test_xmlstream.py, GenericXmlStreamFactoryTests should be named GenericXmlStreamFactoryTestsMixin since it's not a TestCase subclass. It'd be good if its class docstring documented the factory attribute it uses.
2. I would recommend against mixing in two test-defining classes to a single test case. Test method name conflicts on the mixins won't be easily noticed.
3. Test building of protocol. in the docstring for XmlStreamFactoryMixinTest.test_buildProtocolFactoryArguments is probably superfluous.

[ralphm]

Replying to exarkun:

Looking good. 1. in twisted/words/test/test_xmlstream.py, GenericXmlStreamFactoryTests should be named GenericXmlStreamFactoryTestsMixin since it's not a TestCase subclass. It'd be good if its class docstring documented the factory attribute it uses.

Used the suggested name and have it derive from BootstrapMixinTest, which addresses the factory attribute documentation. More below.

1. I would recommend against mixing in two test-defining classes to a single test case. Test method name conflicts on the mixins won't be easily noticed.

Now GenericXmlStreamFactoryTestsMixin derives from BootstrapMixinTest, mixing in two classes is no longer needed. However, trial will want to run its tests by themselves, so I made it test against XmlStreamFactory. Note that this is not a complete test for the latter, since that has the odd args/kwargs thing inherited from XmlStreamFactoryMixin.

1. Test building of protocol. in the docstring for XmlStreamFactoryMixinTest.test_buildProtocolFactoryArguments is probably superfluous.

Fixed.

Please review.

[exarkun]

Thanks. Please merge. :)

[ralphm]

(In [25242]) Let XmlStreamServerFactory not hold onto authenticator instances.

Author: ralphm. Reviewer: exarkun. Fixes #3464.

This splits the code installing bootstrap event observers in a seperate class t.w.x.xmlstream.BootstrapMixin to have t.w.p.j.xmlstream.XmlStreamServerFactory depend on that, instead of t.w.x.xmlstream.XmlStreamFactoryMixin. The former now builds the protocol itself, so the odd dance with the args attribute is not needed.