Opened 5 years ago

Closed 4 years ago

#3834 enhancement closed fixed (fixed)

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

Reported by: itamar Owned by: thijs
Priority: normal Milestone:
Component: core Keywords: documentation, easy
Cc: thijs, mwh, exarkun Branch: branches/examples-reactor-run-3834-2
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

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 5 years ago by exarkun

  • Keywords easy added

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

comment:3 Changed 5 years ago by glyph

  • Owner changed from glyph to thijs

comment:4 Changed 5 years ago by thijs

  • Author set to thijs
  • Branch set to branches/examples-reactor-run-3834

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

comment:5 in reply to: ↑ 2 Changed 5 years ago by thijs

  • Cc thijs added
  • Keywords review added
  • Owner thijs 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 follow-up: Changed 5 years ago by mwh

  • Cc mwh added
  • Keywords review removed
  • Owner set to thijs

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 5 years ago by mwh

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

comment:8 in reply to: ↑ 6 Changed 5 years ago by thijs

  • Keywords review added
  • Owner thijs 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 5 years ago by mwh

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 5 years ago by mwh

  • Keywords review removed
  • Owner set to thijs

I should have moved this out of the review state.

comment:11 Changed 5 years ago by thijs

  • Owner changed from thijs to exarkun

assigning to exarkun for help/advice with this ticket.

comment:12 follow-up: Changed 5 years ago by exarkun

  • Cc exarkun added
  • Owner changed from exarkun to thijs

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 4 years ago by thijs

  • Status changed from new to assigned

comment:14 Changed 4 years ago by thijs

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

comment:15 in reply to: ↑ 12 Changed 4 years ago by thijs

  • Keywords review added
  • Owner thijs deleted
  • Status changed from assigned to new

Replying to exarkun:

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

Merged forward and replaced that call.

comment:16 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to thijs

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 4 years ago by thijs

  • Status changed from new to assigned

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

comment:18 Changed 4 years ago by thijs

  • Branch changed from branches/examples-reactor-run-3834 to branches/examples-reactor-run-3834-2

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

comment:19 Changed 4 years ago by thijs

  • Keywords review added
  • Owner thijs deleted
  • Status changed from assigned to new

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

comment:20 Changed 4 years ago by jerub

  • Owner set to jerub
  • Status changed from new to assigned

comment:21 Changed 4 years ago by jerub

  • Keywords review removed
  • Owner changed from jerub to thijs
  • Status changed from assigned to new

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 4 years ago by <automation>

  • Owner thijs deleted

comment:23 Changed 4 years ago by thijs

  • Owner set to thijs
  • Status changed from new to assigned

Thanks for the review.

comment:24 Changed 4 years ago by thijs

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

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