Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4479 defect closed fixed (fixed)

Documentation: Process implements IProcessTransport; signalProcess accepts "TERM" arg

Reported by: Jason Rennie Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Glyph, jesstess Branch: branches/iprocesstransport-signal-doc-4479
branch-diff, diff-cov, branch-cov, buildbot
Author: jesstess

Description

t.i.Process (or t.i._BaseProcess) should be listed as an IProcessTransport implementation.

IProcessTransport only lists "HUP", "KILL", "STOP", and "INT" as valid cross-platform signals, yet the regular/unix and win32 Process implementations also support "TERM".

See also #2838 and #1478. #1478 also notes that Process should be marked as implementing IProcessTransport.

Attachments (2)

twisted-4479-try1.patch (13.5 KB) - added by Jason Rennie 7 years ago.
Try #1
twisted-4479-try2.patch (1.2 KB) - added by Jason Rennie 7 years ago.
second try

Download all attachments as: .zip

Change History (12)

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

Keywords: easy added
Owner: changed from Glyph to Jason Rennie

Can you provide a patch for this?

Changed 7 years ago by Jason Rennie

Attachment: twisted-4479-try1.patch added

Try #1

comment:2 in reply to:  1 Changed 7 years ago by Jason Rennie

Replying to exarkun:

Can you provide a patch for this?

Just attached. Might be a bit overzealous---I can attach a more lightweight patch if so. I also added IFileDescriptor, IReadDescriptor to PTYProcess; added IConsumer to Process; added IPushProducer to the win32 Process; noted interface methods; and grouped methods by interface.

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

The actual Windows implementation seems to support only INT, TERM, and KILL. So both the original version of the signal list and the new one seem to be wrong. Why shouldn't the docs just say INT, TERM, and KILL?

Also, yes, please limit the patch to fixing the issue described by the ticket. If there are other issues, feel free to file more tickets.

And when you think a ticket is ready for review, add the "review" keyword and re-assign the ticket to no one. See ReviewProcess.

Changed 7 years ago by Jason Rennie

Attachment: twisted-4479-try2.patch added

second try

comment:4 Changed 7 years ago by Jason Rennie

Keywords: review added
Owner: Jason Rennie deleted

"second try" attachment is a minimal patch which adds IProcessTransport implementation to t.i.Process and updates IProcessTransport.signalProcess doc to only list INT, TERM, and KILL as cross-platform signals.

comment:5 Changed 7 years ago by jesstess

Cc: jesstess added
Owner: set to jesstess
Type: enhancementdefect

comment:6 Changed 7 years ago by jesstess

Author: jesstess
Branch: branches/iprocesstransport-signal-doc-4479

(In [29289]) Branching to 'iprocesstransport-signal-doc-4479'

comment:7 Changed 7 years ago by jesstess

Resolution: fixed
Status: newclosed

(In [29291]) Merge iprocesstransport-signal-doc-4479

Author: jrennie Reviewer: jesstess Fixes: #4479

Document that IProcessTransport accepts KILL, TERM, and INT as valid cross-platform signals.

comment:8 Changed 7 years ago by jesstess

Keywords: easy review removed

Thanks for the patch, jrennie!

Process doesn't implement getPeer, which is part of the ITransport interface inherited by IProcessTransport, so I only applied the part of the patch affect IProcessTransport. Please open a separate ticket about the interface, if you'd like.

If Process did end up implementing IProcessTransport it should get a test case for that (grep for verifyObject for examples).

Also note that you don't have to escape quotes in the epytext markup.

comment:9 Changed 7 years ago by Screwtape

Process doesn't implement getPeer, which is part of the ITransport interface inherited by IProcessTransport, so I only applied the part of the patch affect IProcessTransport. Please open a separate ticket about the interface, if you'd like.

Filed as #4585.

comment:10 Changed 7 years ago by <automation>

Owner: jesstess deleted
Note: See TracTickets for help on using tickets.