Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#5705 enhancement closed fixed (fixed)

'twist', a new command-line tool for doing Twisted things without daemonizing

Reported by: Glyph Owned by: Wilfredo Sánchez Vega
Priority: normal Milestone:
Component: core Keywords: twist
Cc: ralphm, chris@…, Alexei Romanoff, Stephen Thorne, Glyph, David Reid Branch: 5705-twist-2
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description (last modified by Glyph)

twistd does log rotation and daemonization, which are sometimes helpful but also sometimes annoying. In many cases, a nicer default behavior is to do what e.g. the 'postgres' binary does; start writing logs (to stderr, not stdout) and don't do much else. This lets a variety of process-management tools (twistd procmon, systemd, launchd, upstart, and supervisord to name a few, although I'm sure there are more) do their own management of log output, to do monitoring of the status of long-running processes, etcetera. Such a default would make it a bit more straightforward to write tools that want to do master/slave multiprocess Twisted setups, since you could easily deal with log output (stderr) at the same time as having a protocol on stdout: which, while possible if you know what you're doing with twistd, is subtle and tricky to get set up properly. Also, this type of default would be much better for client utilities; for example, twist getPage http://example.com/ could be the equivalent of curl http://example.com/.

Once you remove logging and daemonization, it might seem like there's not as much that a tool framework can do for you, since those are often-given examples of why you want to write a twistd plugin. There are still plenty of things that we might want to make such a tool do: reactor selection, for example; running the reactor for you, invoking twistd plugins (and possibly also some other kind of plugin that would run a function that returns a Deferred, stops the reactor after said Deferred fires, and cancel the Deferred on ^C rather than using the Service API), dealing with signal handlers and attaching a root service to reactor system event triggers. It may also be useful to have such a tool optionally do all the same stuff that twistd does, but with better defaults, more suitable for the modern deployment landscape. (For example, 'twist -d' to daemonize).

Another use-case that would be interesting to address with this tool would be to make its invocations mirror the way someone would use 'python' for developing a blocking program. When one is writing a python program, one generally starts off noodling around at a shell prompt, putting together little expressions at the interactive interpreter, defining functions. Then, when re-defining that function gets too tedious, one puts it into a little script, and starts alternating between editing it and running 'python my-little-script.py'. Finally, one takes the relevant code and pastes it into a real module (or, ideally, a test suite). Doing this with Twisted right now is tedious, because in order to set up basic scaffolding to run the reactor, you have to import a whole bunch of names and handle errors from Deferreds and make sure not to double-stop the reactor and so on.

My suggestion would be for twist to run something like python -m twisted.conch.stdio when you run it interactively, so you can get started with little non-blocking things with Deferreds. The next step would be to run twist some-filename.py; at this point I'm not exactly sure what that would do with Deferreds; maybe you'd need to define a function and it would run task.react for you, or maybe it would treat the whole file as the body of an inlineCallbacks generator (although that's syntactically invalid python), or maybe we would parse the file line by line in way similar to the way the interactive interpreter does, pausing for Deferred results at the top level. These all have their drawbacks, but the idea is, you shouldn't have to run the reactor yourself. Finally you can put your code into a plugin or run it under trial, or otherwise treat it as more production worthy; we don't need any support from twist in that final step.

Change History (27)

comment:1 Changed 7 years ago by ralphm

Cc: ralphm added

comment:2 Changed 7 years ago by Christian Kampka

Cc: chris@… added

comment:3 Changed 7 years ago by Alexei Romanoff

Cc: Alexei Romanoff added

comment:4 Changed 7 years ago by Glyph

Description: modified (diff)

comment:5 Changed 7 years ago by Glyph

Cc: Stephen Thorne Glyph David Reid added

comment:6 Changed 7 years ago by Glyph

Description: modified (diff)

Adding some stuff to the description.

comment:7 Changed 7 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:8 Changed 4 years ago by Glyph

Since the whole point of twist is to get away from a legacy style of doing things and learn from as many mistakes as possible, it should also make a point of defaulting to structured log output, i.e. twisted.logger.jsonFileLogObserver. Human readable logs are a bad idea, because almost by definition, nobody reads every line of a log; you search for stuff looking for specific log types or specific timestamps; right now 'grep' is a standard tool for this, but increasingly, you ingest stuff into a database first. We should optimize for the "good" case, where we make ingesting into a database as easy as possible. We can also have a Twisted log viewer for presenting nicer human readable messages once you've pulled the JSON blob out of some kind of database (this is what eventsFromJSONLogFile and formatEvent in twisted.logger are for).

Last edited 4 years ago by Glyph (previous) (diff)

comment:9 Changed 3 years ago by Wilfredo Sánchez Vega

Owner: changed from Glyph to Wilfredo Sánchez Vega
Status: assignednew

Stealing this for 2016 PyCon Sprints

comment:10 Changed 3 years ago by Wilfredo Sánchez Vega

Status: newassigned

comment:11 Changed 3 years ago by Wilfredo Sánchez Vega

Branch: 5705-twist
Keywords: review added
Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

comment:12 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to Wilfredo Sánchez Vega
Status: newassigned

comment:13 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review added
Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

OK, I think I have a working implementation.

Tests are passing in buildbot, with what I think are some false positives in twistedchecker. There are three groups of these:

  1. The first letter of comment should be capitalized: The first word is a symbol and should not, in fact, be capitalized. See also https://github.com/twisted/twistedchecker/issues/103.
  1. Complaints against twisted.application.twist._options: The docstrings here are used by usage.Options and shown to users, so @param noise is not welcome here.
  1. twisted.test.proto_helpers.MemoryReactor: There are few methods noted as having new issues here which I didn't touch. The latter few I did, but the missing parameters are defined in IReactorCore. I'm not sure how to fix this other than copying the params, which seems dumb, or to delete the docstrings here altogether, which may be dumber.

Finally, there is: ` twisted.application.runner._runner.Runner.killIfRequested: invalid ref to ExitStatus.EX_USAGE `

ExitStatus is imported in that file, and EX_USAGE is an attribute thereof... so I'm not sure why that's an invalid ref.

comment:14 Changed 3 years ago by Wilfredo Sánchez Vega

Adding @cvar docs for EX_* fixed the pydoctor issue.

comment:15 Changed 3 years ago by Craig Rodrigues

Keywords: review removed
Owner: set to Wilfredo Sánchez Vega

Thanks for the submission. This patch does not have 100% diff coverage according to: https://codecov.io/gh/twisted/twisted/compare/trunk...5705-twist

That is now being enforced for all new patches to Twisted. Can you add a test to bump this up to 100%?

comment:16 Changed 3 years ago by Glyph

Hi rodrigc - a little bit of meta-review feedback here; you didn't say what to do next. If you have no other feedback, and you like the implementation here, a full review (removal of the 'review' keyword) should say something like 'add a test to bump coverage to 100%, then merge', given that wsanchez is a committer. Do you think this should be re-submitted for more feedback? Do you have any non-mandatory critique or feedback on the implementation?

As always: thanks for doing reviews, please do not take this feedback to mean you should do fewer in the future :).

comment:17 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review added

OK some changes:

First, I made the runner API private for now. Glyph had some ideas for improving it, so let's avoid compat problems until we flush that out. In particular, Runner can be separated out a bit, like we may want a PIDFile class.

Anyway, this still exposes the new CLI tool.

The coverage was cleaned up significantly. What left I'd like to punt on:

  • Some of _exit.py isn't covered because that code is there for platforms that lack the posix module. It's just definitions…
  • Some of proto_helpers isn't covered. This is a test class that I added IReactorCore API to. Instead of implementing just some of the API, I filled in the rest but left some raising NotImplementedError in case some new tests try to call it. These could be removed, but I think this is more explicit. Tests of tests aren't exactly typical, anyway.

Putting back up for review, will need file tickets after this one is done:

  • Glyph's suggested twisted.conch.stdio feature.
  • Cleaning up the runner API so we can make it public.

comment:18 Changed 3 years ago by Wilfredo Sánchez Vega

Owner: Wilfredo Sánchez Vega deleted

comment:19 Changed 3 years ago by Craig Rodrigues

Keywords: review removed
Owner: set to Wilfredo Sánchez Vega

Please incorporate the following changes:

(1) Delete bin/twist from your patch. Instead, add an entry point to

*portedToPython3Scripts* https://github.com/twisted/twisted/blob/trunk/twisted/python/dist.py#L229

We are now using console scripts

(2) In your topfile, combine the text onto one really long line. Apparently

the script which parses topfiles cannot handle newlines, so everything has to be on one line.

Do those two things, and you are good to go, merge it!

comment:20 Changed 3 years ago by Wilfredo Sánchez Vega

Branch: 5705-twist5705-twist-2

Merged forward to 5705-twist-2, required to add to portedToPython3Scripts.

On that branch, I

  • Added twist to portedToPython3Scripts
    • Did some linting on dist.py while I was in there.
  • removed bin/twist
  • Put all of the topfile on one line.

comment:21 Changed 3 years ago by Wilfredo Sánchez Vega

Also: I made the Twist class private for now.

comment:22 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: twist added

comment:23 Changed 3 years ago by Wilfredo Sánchez Vega

Also added new modules top dist3.py and fixed a small amount of string nonsense to make py3 happy.

comment:24 Changed 3 years ago by Wilfredo Sánchez Vega

Resolution: fixed
Status: newclosed

branch merged

comment:25 Changed 3 years ago by Wilfredo Sánchez Vega

Follow-ups:

  • #8656: twist --deamon
  • #8657: python -m twisted
  • #8658: Clean up twist and runner API so as to make them public

comment:26 Changed 3 years ago by Jean-Paul Calderone

How about another follow-up for adding documentation?

comment:27 Changed 3 years ago by Wilfredo Sánchez Vega

There's a ticket open for that, #8942.

In the meantime, there is twist --help for top-level options. The plugins work the same as twistd.

Note: See TracTickets for help on using tickets.