Opened 4 years ago

Closed 4 years ago

#7083 enhancement closed fixed (fixed)

Update echoserv_ssl.py to use react

Reported by: Jean-Paul Calderone Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Mike Cooper Branch: branches/echoserv-ssl-react-7083
branch-diff, diff-cov, branch-cov, buildbot
Author: hynek

Description

"Single shot" clients like this one - which perform some well-defined operation with a clear completion condition - should use twisted.internet.task.react.

Attachments (2)

echoserv-react.patch​ (4.1 KB) - added by Mike Cooper 4 years ago.
echoserv-react.2.patch​ (3.7 KB) - added by Mike Cooper 4 years ago.
Version 2

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by Mike Cooper

Cc: Mike Cooper added
Owner: set to Mike Cooper
Status: new → assigned

comment:2 in reply to:  description Changed 4 years ago by Mike Cooper

Replying to exarkun:

"Single shot" clients like this one - which perform some well-defined operation with a clear completion condition - should use twisted.internet.task.react.

In the subject you said "Update echoserv_ssl.py", but in the description you said "clients like this". Are you talking about "echoclient_ssl.py" instead?

Changed 4 years ago by Mike Cooper

Attachment: echoserv-react.patch​ added

comment:3 Changed 4 years ago by Mike Cooper

Keywords: review added
Owner: Mike Cooper deleted
Status: assigned → new

Assuming that the point of this was to fix the clients, I did that.

comment:4 Changed 4 years ago by habnabit

Keywords: review removed
Owner: set to Mike Cooper

Thanks for the contribution! There's a few minor issues with it.

  1. You appear to have an extra bit in your diff that's not related to the fix, patching _sslverify.
  2. All changes need a news file.
  3. You call self.done.errback(None), which will fail, because there is no current exception when clientConnectionFailed is called. You might mean instead self.done.callback(None) or self.done.errback(reason)

Changed 4 years ago by Mike Cooper

Attachment: echoserv-react.2.patch​ added

Version 2

comment:5 Changed 4 years ago by Mike Cooper

Keywords: review added
Owner: Mike Cooper deleted

Fixed issues pointed out in comment 4.

comment:6 Changed 4 years ago by Adi Roiban

Latest patch looks good to merge. Thanks!

comment:7 Changed 4 years ago by Hynek Schlawack

Author: → hynek
Branch: → branches/echoserv-ssl-react-7083

(In [43346]) Branching to echoserv-ssl-react-7083.

comment:8 Changed 4 years ago by Hynek Schlawack

(In [43350]) Apply mythmon's patch

At least the parts that still apply, echoclient_ssl.py is using react already.

refs #7083

comment:9 Changed 4 years ago by Hynek Schlawack

Owner: set to hawkowl

Turns out™ echoclient_ssl.py has already been ported over, but echoclient.py itself not.

So I applied what worked and added some prettifications to it to adhere to our standards. Keeping it in review, so someone (an owl volunteered) can double-check my changes.

comment:10 Changed 4 years ago by hawkowl

Keywords: review removed

LGTM, please merge.

comment:11 Changed 4 years ago by Hynek Schlawack

Resolution: → fixed
Status: new → closed

(In [43355]) Merge echoserv-ssl-react-7083

Author: mythmon Reviewers: habnabit, hawkowl Fixes: #7083

The echoclient example now uses twisted.internet.task.react.

Note: See TracTickets for help on using tickets.