Opened 8 years ago

Closed 6 years ago

#3834 enhancement closed fixed (fixed)

TCP client howto missing reactor.run() in one code sample

Reported by: Itamar Turner-Trauring Owned by: Thijs Triemstra
Priority: normal Milestone:
Component: core Keywords: documentation, easy
Cc: Thijs Triemstra, Michael Hudson-Doyle, Jean-Paul Calderone Branch: branches/examples-reactor-run-3834-2
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs

Description

In the "Simple, single-use clients" section there's a full example, but it's missing a reactor.run().

Change History (24)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Keywords: easy added

comment:2 Changed 8 years ago by Jean-Paul Calderone

http://twistedmatrix.com/projects/core/documentation/howto/rdbms.html#auto5 is missing it too. I bet lots of other docs are missing it, too.

comment:3 Changed 8 years ago by Glyph

Owner: changed from Glyph to Thijs Triemstra

comment:4 Changed 8 years ago by Thijs Triemstra

Author: thijs
Branch: branches/examples-reactor-run-3834

(In [27112]) Branching to 'examples-reactor-run-3834'

comment:5 in reply to:  2 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review added
Owner: Thijs Triemstra deleted

Replying to exarkun:

http://twistedmatrix.com/projects/core/documentation/howto/rdbms.html#auto5 is missing it too. I bet lots of other docs are missing it, too.

Seems this is missing more than just as a reactor.run() call but also the imports, which is true for all of the examples on that page. If you can fix these imports, please do. Putting up the changes for the original ticket description for review.

comment:6 Changed 8 years ago by Michael Hudson-Doyle

Cc: Michael Hudson-Doyle added
Keywords: review removed
Owner: set to Thijs Triemstra

I'm not sure adding reactor.run() to the example in clients.xhtml without adding a reactor.stop() as well. It's also a slightly odd example. Do you think changing it to this:

def gotProtocol(p):
    p.sendMessage("Hello")
    defer.maybeDeferred(p.transport.loseConnection).addCallback(reactor.stop)

would make more sense?

All the other changes look good.

comment:7 Changed 8 years ago by Michael Hudson-Doyle

Oops, I forgot something: thanks for working on this!

comment:8 in reply to:  6 Changed 8 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replying to mwh:

I'm not sure adding reactor.run() to the example in clients.xhtml without adding a reactor.stop() as well. It's also a slightly odd example. Do you think changing it to this:

def gotProtocol(p):

p.sendMessage("Hello") defer.maybeDeferred(p.transport.loseConnection).addCallback(reactor.stop)

would make more sense?

Well, if I do that and run the example it throws this error:

Unhandled error in Deferred:
Traceback (most recent call last):
Failure: twisted.internet.error.ConnectionRefusedError: Connection was refused by other side: 61: Connection refused.

Any idea?

comment:9 Changed 8 years ago by Michael Hudson-Doyle

Well, the existing code does that if nothing is listening.

I'm having a hard time getting this correct, so I don't know if I'm a good person to review this branch :) I've got as far as this:

from twisted.internet import reactor
from twisted.internet.protocol import Protocol, ClientCreator
from twisted.python import log

class Greeter(Protocol):
    def sendMessage(self, msg):
        self.transport.write("MESSAGE %s\n" % msg)

def gotProtocol(p):
    p.sendMessage("Hello")
    return p.transport.loseConnection()

c = ClientCreator(reactor, Greeter)
d = c.connectTCP("localhost", 1234)
d.addCallback(gotProtocol).addErrback(log.err).addCallback(
    lambda ignored: reactor.stop())
reactor.run()

But the reactor.stop() gets called before the data is sent.

comment:10 Changed 8 years ago by Michael Hudson-Doyle

Keywords: review removed
Owner: set to Thijs Triemstra

I should have moved this out of the review state.

comment:11 Changed 7 years ago by Thijs Triemstra

Owner: changed from Thijs Triemstra to Jean-Paul Calderone

assigning to exarkun for help/advice with this ticket.

comment:12 Changed 7 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Owner: changed from Jean-Paul Calderone to Thijs Triemstra

Since the code is already doing arbitrary stuff with delayed calls, I think you just want to call reactor.stop after some arbitrary delay that's long enough to let the message be sent most of the time.

That, or make it actually work right, and put the reactor.stop call in the protocol's connection lost (which defeats the purpose of the example, which seems to be about using protocol instances from code that's not part of the protocol class).

So yea, reactor.callLater(3, reactor.stop).

comment:13 Changed 7 years ago by Thijs Triemstra

Status: newassigned

comment:14 Changed 6 years ago by Thijs Triemstra

(In [30553]) correct stop call, add news file. refs #3834

comment:15 in reply to:  12 Changed 6 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

Replying to exarkun:

So yea, reactor.callLater(3, reactor.stop).

Merged forward and replaced that call.

comment:16 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Thijs Triemstra

There's a conflict in clients.xhtml now. Can you merge forward and resolve it? Also, the news file doesn't reflect the style described on ReviewProcess#Newsfiles. Can you re-arrange it so it does, and mention the udp and client howtos specifically?

Thanks.

comment:17 Changed 6 years ago by Thijs Triemstra

Status: newassigned

Thanks for the review, i'll look into it.

comment:18 Changed 6 years ago by Thijs Triemstra

Branch: branches/examples-reactor-run-3834branches/examples-reactor-run-3834-2

(In [30594]) Branching to 'examples-reactor-run-3834-2'

comment:19 Changed 6 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

Applied the changes in a new branch. Doc builder is happy.

comment:20 Changed 6 years ago by Stephen Thorne

Owner: set to Stephen Thorne
Status: newassigned

comment:21 Changed 6 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Stephen Thorne to Thijs Triemstra
Status: assignednew

I've applied this branch to my working copy, read through the examples in the two affected files and while 2 of the examples are runnable, the third simply has an ellipsis and is not runnable - I cut+paste what was required from the other example to splice together something that would work, and it did.

What I'm saying is, I'm glad these examples have been improved, I've tested them, and they work, please merge.

comment:22 Changed 6 years ago by <automation>

Owner: Thijs Triemstra deleted

comment:23 Changed 6 years ago by Thijs Triemstra

Owner: set to Thijs Triemstra
Status: newassigned

Thanks for the review.

comment:24 Changed 6 years ago by Thijs Triemstra

Resolution: fixed
Status: assignedclosed

(In [30759]) Added missing reactor.run() calls in the UDP and client howto documents.

Author: thijs Reviewer: mwh, exarkun, jerub Fixes: #3834

Note: See TracTickets for help on using tickets.