Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4761 defect closed fixed (fixed)

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

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: jesstess Branch: branches/explicit-buildProtocol-4761
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

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).

Change History (19)

comment:1 Changed 4 years ago by glyph

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

comment:2 Changed 4 years ago by itamar

The server howto doesn't even mention buildProtocol!

comment:3 Changed 4 years ago by glyph

  • Owner changed from glyph to itamar

Indeed not.

comment:4 Changed 4 years ago by exarkun

  • Keywords easy removed

comment:5 Changed 4 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/explicit-buildProtocol-4761

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

comment:6 Changed 4 years ago by itamar

  • Summary changed from Documentation should use explicit buildProtocol()s rather than magic "protocol = MyProtocol" on factories to Server documentation should use explicit buildProtocol()s rather than magic "protocol = MyProtocol" on factories

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.

comment:7 Changed 4 years ago by itamar

  • Keywords review added
  • Owner itamar deleted

Ready for review.

comment:8 Changed 4 years ago by jerub

  • Keywords review removed
  • Owner set to itamar

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.

comment:9 Changed 4 years ago by 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.

  1. Changed connection counter to not be used to reject connections.
  2. 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

comment:10 Changed 4 years ago by itamar

  • Keywords review added
  • Owner itamar deleted

comment:11 Changed 4 years ago by jesstess

  • Owner set to jesstess

comment:12 Changed 4 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to itamar

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!

comment:13 follow-up: Changed 4 years ago by 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.

comment:14 in reply to: ↑ 13 Changed 4 years ago by 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.

comment:15 follow-up: Changed 4 years ago by 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.

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

comment:17 Changed 4 years ago by exarkun

(In [31668]) Address review feedback

refs #4761

comment:18 Changed 4 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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.

comment:19 in reply to: ↑ 15 Changed 4 years ago by 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.

Note: See TracTickets for help on using tickets.