Server documentation should use explicit buildProtocol()s rather than magic "protocol = MyProtocol" on factories

[itamar]

Observation at the Twisted Lab in Boston suggests that buildProtocol as an important protocol/factory integration point is hard for new users to understand. This seems to be at least in part because the documentation tends to use the "protocol = MyProtocol" shortcut, which is basically magic and hides the existence buildProtocol from new developers.

All uses of factories in our documentation ought to use an explicit buildProtocol. The client and server howtos should document "protocol = MyProtocol" (but not use it otherwise).

[glyph]

+1. I found that the explanation of buildProtocol as a method went over much more easily than the 'convenience' syntax of protocol = MyProtocol.

[itamar]

The server howto doesn't even mention buildProtocol!

[glyph]

Indeed not.

[itamarst]

(In [31547]) Branching to 'explicit-buildProtocol-4761'

[itamar]

I've decided to restrict this ticket to the server howto, since I'm running out of steam. I'll open a separate ticket for the client howto.

[itamar]

Ready for review.

[jerub]

Thanks for this patch!

There is a simple error in the chat.py script. 'self.transport.write' is used instead of self.sendLine(). sendLine is much preferred because explicit line endings aren't required, and if the line endings change, what's transmitted is the same as what's received.

I dislike the part about how to limit the number of connections to the server. It's poor form for a network server to accept a connection, decide it didn't want to accept the connection, and drop the connection, instead buildProtocol fail and the connection should never be accepted. If you (or anyone else reading this review) agree this is the case, please file a ticket, as I think that's outside the scope of this enhancement.

There is a word in this document 'dynamicity', which is not in my spell checker, and perhaps this sentence could be phrased better. See: "Use Python's dynamicity to create open ...".

I have read through the changes to the prose and agree that it reads better and is clearer now, and I thoroughly appreciate the rewrite to get rid of the f = Factory(); f.protocol = MyProtocol idiom. Thank you!

I have manually checked the lore output, and it looks good, and there are no lint errors.

Please action the following:

1. update the chat.py example and test it manually.
2. file a ticket for the too many connections issue I raised, but only if you think it's worth changing (reference the number here if you did.
3. Reword the dynamicity sentence.
4. resubmit for review.

[itamarst]

(In [31589]) Address review comments:

1. Replace transport.write with sendLine in chat example, and tested manually. Also added sentence explaining sendLine where it is first introduced. 2. Changed connection counter to not be used to reject connections. 3. Got rid of confusing sentence involving dynamicity; perhaps one day we'll write full howto on state machines, but this can't be explained one sentence.

References #4761

[jesstess]

Thanks for working on this, itamar. A few comments:

• I don't understand what the "which" is in "What this means is that the protocol responds to events as they arrive from the network, which arrives by calling functions on the protocol."
• "the exists" ==> "that exists" in

+    instance. The factory is used to share state the exists beyond the
+    lifetime of any given connection. You will see why this object is

• The example in "Simpler Protocol Creation" uses listenTCP, but listenTCP is only explained after its second use, in listings/servers/chat.py.
• If the hope is that readers will put together the two bits of QOTD code from the Protocols section and have something runnable, some imports are missing:

from twisted.internet.protocol import Factory
from twisted.internet.endpoints import TCP4ServerEndpoint
from twisted.internet import reactor

• The changes need a topfile.
• I couldn't find the ticket for the client howto. Can you link it here?

Other than that, I think this is good to merge!

[itamar]

Further work will be done when I figure out how to get open source IP release from Google (this may take a couple of weeks), unless someone else wants to take this up.

[glyph]

Replying to itamar:

Further work will be done when I figure out how to get open source IP release from Google (this may take a couple of weeks), unless someone else wants to take this up.

If / when you get that, please alert us if you will be contributing under Google's copyright or your own, so we know if we need to modify the copyright holder list in LICENSE.

[exarkun]

It's poor form for a network server to accept a connection, decide it didn't want to accept the connection, and drop the connection, instead buildProtocol fail and the connection should never be accepted.

This is wrong. First, you cannot see where the connection is from until you accept it. Second, if you don't accept it, the client is left waiting for a timeout before they know what's going on. It's better to disconnect them immediately. Best is to provide a reason for disconnecting them, like 503 Service Unavailable, but if this isn't possible actively accepting and then disconnecting still tells them the problem is with the server they are trying to connect, not some random intermediate router.

[exarkun]

Thanks for the review jesstess.

I don't understand what the "which"

Fixed

"the exists" ==> "that exists"

Fixed

The example in "Simpler Protocol Creation" uses listenTCP

I changed it to use an endpoint, like the earlier example. I was tempted to make a similar change to chat.py, but that would have left the document with no examples of listenTCP which I guess' would be bad.

If the hope is that readers will put together the two bits of QOTD

I added the missing imports.

The changes need a topfile.

Looks like someone (itamar?) got to that first.

I couldn't find the ticket for the client howto. Can you link it here?

It's #5044

[exarkun]

(In [31668]) Address review feedback

refs #4761

[exarkun]

(In [31670]) Merge explicit-buildProtocol-4761

Author: itamar Reviewer: jesstess Fixes: #4761

In the server howto, cover Factory.buildProtocol before introducing the Factory.protocol shortcut. This should make it more clear to new users where protocols come from and what their relationship to factories is.

[glyph]

Replying to exarkun:

It's poor form for a network server to accept a connection, decide it didn't want to accept the connection, and drop the connection, instead buildProtocol fail and the connection should never be accepted.

This is wrong. First, you cannot see where the connection is from until you accept it.

To clarify further, by the time buildProtocol is being called, the connection has been accept()ed. There's no way to avoid it. See the implementation here. The reason buildProtocol is allowed to return None is to avoid allocation work on an overloaded server on the python side of things, not to change the behavior at the networking layer.