Rename Process kill methods to signal #8642

jan-zajic · 2020-01-02T15:18:52Z

As suggested by @ysbaddaden in #8518 and further discussed in draft #8536 rename Process#kill(Signal) to Process#signal(Signal) that we can eventually implement on Windows (if needed) with a deprecation notice. The name kill is confusing as it can means two things - send signal to process, or send signal kill (9) to process when argument ommited. But currently default behavior of kill method without providing signal argument is to send term signal to process (simulates posix behavior of kill command, but we need to remove POSIXisms of apis to become realy portable imho).

bcardiff · 2020-01-02T15:23:50Z

Please keep Process#kill with a @[Deprecated("Use #signal instead")] annotation.
And keep a minimal spec that uses #kill

Blacksmoke16 · 2020-01-02T15:44:36Z

I'd also advocate for including something like, and will be removed in Crystal 0.34.0 so that there is an actual expectation of when it will/should be removed.

spec/std/process_spec.cr

jan-zajic · 2020-01-02T16:07:19Z

Either way i think it's better that renamed signal method should not have default value, so i removed it. Because of code readability - with process.signal without argument it's very unclear what it does.

jan-zajic · 2020-01-02T16:17:08Z

btw @bcardiff build now fail on some crazy error (in more PRs, not only this):

.build/std_spec: relocation error: .build/std_spec: symbol gmon_start version GLIBC_2.2.5 not defined in file libpthread.so.0 with link time reference

bcardiff · 2020-01-02T17:00:30Z

@jan-zajic Yeah, it should be fixed by #8641

src/process.cr

Co-Authored-By: Benoit de Chezelles <bew@users.noreply.github.com>

jan-zajic · 2020-01-03T13:43:46Z

@RX14 , @straight-shoota updated according to reviews.

src/process.cr

straight-shoota

The changes from process.kill to process.signal Signal::Term are somewhat counterproductive.
#kill without arguments is going to become #terminate in #8536.

I'd suggest to introduce #terminate and #interrupt already in this PR. It makes sense because they're portable successors to #kill and the different uses can be replaced appropriately.

As a bonus, #8536 would truly be only a refactor and neither add nor change any features.

straight-shoota · 2020-01-06T17:22:21Z

src/compiler/crystal/tools/playground/server.cr

@@ -161,7 +161,7 @@ module Crystal::Playground
        @logger.info "Code execution killed (session=#{@session_key}, filename=#{@running_process_filename})."
        @process = nil
        File.delete @running_process_filename rescue nil
-        process.kill rescue nil
+        process.signal(::Signal::TERM) rescue nil


straight-shoota · 2020-01-06T17:22:35Z

src/process.cr

@@ -193,7 +199,7 @@ class Process
      $? = process.wait
      value
    rescue ex
-      process.kill
+      process.signal Signal::TERM


And this one.

Co-Authored-By: Benoit de Chezelles <bew@users.noreply.github.com>

jan-zajic · 2020-01-08T12:07:36Z

@straight-shoota I'm all for it. But that assumes we will agree on names of that methods. I still don't know any opinion of others. I'am currently for #terminate and #destroy as shown in new commit.

The options considered so far are (in order gracefully, force shutdown):

#terminate_gracefully, #terminate_immediately
#terminate, #kill
#terminate, #interrupt
#terminate, #destroy

So please vote! @straight-shoota, @RX14, @ysbaddaden, @j8r, @Sija, @bew, @bcardiff ping. If I forgot someone please let them know.

straight-shoota · 2020-01-08T13:26:01Z

The method to terminate a process is commonly named kill in most APIs, which is named after the homonymous unix program.
I think it's best to follow that line and keep the name. The existing overload of #kill without arguments can stay, only the behaviour should be changed to send SIGKILL on Posix. This is a breaking change, but not very relevant.

Let's not bother with an alternative method for graceful termination in this PR. Having separate methods for sending signals and terminating a process is sufficient. If you need graceful termination, you can easily send SIGTERM manually. Whether we should have a dedicated method for this should be a separate discussion to not prolong this PR even more.

Sija · 2020-01-08T13:31:13Z

kill is a commonly used term as @straight-shoota mentioned, so my vote goes for:

#terminate, #kill

j8r · 2020-01-08T14:36:37Z

The kill man is: kill - send a signal to a process.

I prefer Process#send_signal.

RX14 · 2020-01-08T15:31:25Z

Please let's not use kill, it's confusing with the existing API.

signal should be used for sending signals

terminate should terminate the process

interrupt will interrupt the process and ask it to shut down

i like the current naming, and i'd prefer for it to stay

Sija · 2020-01-08T15:40:22Z

Please let's not use kill, it's confusing with the existing API.

@RX14 How come? It sends KILL signal, as you would expect from the naming...

straight-shoota · 2020-01-08T15:40:59Z

it's confusing with the existing API.

I wouldn't worry about that. We need to focus on the best API for the future.
It seems literally everyone names this "kill", so I doubt we should settle for something else.

RX14 · 2020-01-08T18:38:30Z

No, kill is confusingly for sending signals, with the default being SIGTERM.

So would you rather have kill send SIGTERM and terminate send SIGKILL?

RX14 · 2020-01-08T18:39:26Z

Actually this is all too confusing the only interface should be #signal as I originally said.

Sija · 2020-01-08T19:04:05Z

@RX14 What's confusing about calling Process#kill, which in turn would send Signal::KILL signal...? For my taste it can't be clearer than that.

Process#terminate => Process#signal(Signal::TERM)
Process#kill => Process#signal(Signal::KILL)

straight-shoota · 2020-01-08T22:56:35Z

No, kill is confusingly for sending signals, with the default being SIGTERM.

The default should be SIGKILL. Even if it was SIGTERM, that's merely an implementation detail.
Process#kill just provides a generic, high level interface to terminate a process. The exact implementation and behaviour is obviously platform specific, but we should try to make it as similar as possible across platforms.

Simply having everyone call signal(:kill) is not sufficient. It just mimics the POSIX API and is neither generic nor idiomatic. There is an explicit method to spawn a process (which is already at a very high level of abstraction) and there should also be an explicit method to terminate a process at the same level of abstraction.
(We can certainly make signal(:kill) work on windows as well)

RX14 · 2020-01-09T11:33:03Z

We can't use the #kill name right now because #kill with no args means SIGTERM.

There must be at least one release where #kill is removed, before we reuse it's name with a different semantics.

RX14 · 2020-01-09T11:34:28Z

How about introducing just #terminate in this PR which sends SIGTERM, then after #kill is gone, we can add it back sending SIGKILL. So this PR would just have #signal and #terminate?

jan-zajic · 2020-01-09T12:01:31Z

That sounds reasonable to me. čt 9. 1. 2020 v 12:34 odesílatel Stephanie Hobbs <notifications@github.com> napsal:

…

How about introducing just #terminate in this PR which sends SIGTERM, then after #kill is gone, we can add it back sending SIGKILL. So this PR would just have #signal and #terminate? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8642?email_source=notifications&email_token=ADLYAP55TGUDT2L6FUSUCF3Q44DULA5CNFSM4KCDMAHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIP75LQ#issuecomment-572522158>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLYAPYXMR7ATM47QVLPKRTQ44DULANCNFSM4KCDMAHA> .

straight-shoota · 2020-01-09T12:14:08Z

Sure, we can do that. But IMO we don't need a full deprecation cycle just to change the implementation of Process#kill from SIGTERM to SIGKILL. That seems to be an overkill. The method still works the same way terminating a process. It only makes a difference when the process actually traps SIGTERM.

However I'm still not sure we actually want a #terminate method for signalling graceful termination.

RX14 · 2020-01-10T11:27:54Z

spec/std/process_spec.cr

+
+    process.terminate
+    process.exists?.should be_true
+    process.terminated?.should be_false


This seems like a race condition to me. If the program gets rescheduled between the terminate call and the exists? check then this spec will fail. I suspect that running this test in a loop on a loaded machine will already fail.

I'm not sure there's an appropriate way to test this.

RX14 · 2020-01-10T11:29:24Z

This is OK for me to merge except for the race condition in the spec suite

straight-shoota · 2020-02-18T15:26:26Z

@jan-zajic Are you still up for this addressing the above comment?

jan-zajic · 2020-02-26T13:46:31Z

@straight-shoota ok i look at it in few days

oprypin · 2020-04-03T14:40:27Z

Was write access given to the branch? Could someone simply remove this spec and merge the PR?

oprypin · 2020-04-03T14:55:11Z

Mhm, the merge is non-trivial.

I have merged it and I'm ready to send a new PR.
master...oprypin:winkill

I think such a PR, if merged, would show me as the primary author, which is not ideal.

oprypin · 2020-04-03T14:55:27Z

@jan-zajic, could you give some input?
Would you be able to keep pushing this forward?
Do you want to hand over the ownership of this somehow?
Are you OK with someone else re-creating a pull request with your commits?

jan-zajic · 2020-04-08T08:53:18Z

@oprypin unfortunately i don't have time right now to be able to keep pushing this forward.. So i'm ok with your help, as long as you keep my original work (to some point that make sense), so my original effort will not be wasted.. Last time i put a lot of effort into this, but but we got lost in discussions that led nowhere.

jan-zajic · 2020-04-08T08:54:02Z

Closed in favor of #9006

jan-zajic mentioned this pull request Jan 2, 2020

Extract POSIX implementation of Process and Signal to a portable API #8536

Closed

ysbaddaden reviewed Jan 2, 2020

View reviewed changes

spec/std/process_spec.cr Outdated Show resolved Hide resolved

jan-zajic force-pushed the feature/rename_kill_signal branch from 126f89f to efb4f11 Compare January 2, 2020 16:03

jan-zajic force-pushed the feature/rename_kill_signal branch from efb4f11 to 9dfa84b Compare January 2, 2020 16:09

RX14 requested changes Jan 2, 2020

View reviewed changes

src/process.cr Show resolved Hide resolved

src/process.cr Outdated Show resolved Hide resolved

straight-shoota reviewed Jan 2, 2020

View reviewed changes

src/process.cr Outdated Show resolved Hide resolved

src/process.cr Outdated Show resolved Hide resolved

straight-shoota added breaking-change kind:refactor topic:stdlib:system labels Jan 2, 2020

jan-zajic added a commit to jan-zajic/crystal that referenced this pull request Jan 3, 2020

crystal-lang#8642 requested changes

8c705bf

jan-zajic requested a review from RX14 January 3, 2020 13:01

bew reviewed Jan 3, 2020

View reviewed changes

src/process.cr Outdated Show resolved Hide resolved

jan-zajic added a commit to jan-zajic/crystal that referenced this pull request Jan 3, 2020

crystal-lang#8642 style update to avoid confusion

81e2973

Co-Authored-By: Benoit de Chezelles <bew@users.noreply.github.com>

RX14 approved these changes Jan 3, 2020

View reviewed changes

src/process.cr Outdated Show resolved Hide resolved

straight-shoota reviewed Jan 6, 2020

View reviewed changes

jan-zajic and others added 5 commits January 8, 2020 10:44

rename Process#kill methods to signal (make old ones deprecated)

99327b5

remove default value of Process#signal method argument

c2c417e

crystal-lang#8642 requested changes

7c499d5

add an explicit : Nil return type to the signal function

650b67f

crystal-lang#8642 style update to avoid confusion

c0ed6fb

Co-Authored-By: Benoit de Chezelles <bew@users.noreply.github.com>

jan-zajic force-pushed the feature/rename_kill_signal branch from b75ea9b to 6149b46 Compare January 8, 2020 12:07

portable API to stop Process

ee61fb5

jan-zajic force-pushed the feature/rename_kill_signal branch from 6149b46 to ee61fb5 Compare January 10, 2020 08:56

RX14 requested changes Jan 10, 2020

View reviewed changes

oprypin mentioned this pull request Apr 5, 2020

Rename Process kill methods to signal [by @jan-zajic] #9006

Merged

jan-zajic closed this Apr 8, 2020

oprypin mentioned this pull request Apr 10, 2020

Coordinate porting to Windows #5430

Open

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Process kill methods to signal #8642

Rename Process kill methods to signal #8642

jan-zajic commented Jan 2, 2020

bcardiff commented Jan 2, 2020

Blacksmoke16 commented Jan 2, 2020 •

edited

Loading

jan-zajic commented Jan 2, 2020

jan-zajic commented Jan 2, 2020

bcardiff commented Jan 2, 2020

jan-zajic commented Jan 3, 2020

straight-shoota left a comment

straight-shoota Jan 6, 2020

straight-shoota Jan 6, 2020

jan-zajic commented Jan 8, 2020 •

edited

Loading

straight-shoota commented Jan 8, 2020

Sija commented Jan 8, 2020

j8r commented Jan 8, 2020

RX14 commented Jan 8, 2020 •

edited

Loading

Sija commented Jan 8, 2020 •

edited

Loading

straight-shoota commented Jan 8, 2020

RX14 commented Jan 8, 2020

RX14 commented Jan 8, 2020

Sija commented Jan 8, 2020 •

edited

Loading

straight-shoota commented Jan 8, 2020 •

edited

Loading

RX14 commented Jan 9, 2020

RX14 commented Jan 9, 2020

jan-zajic commented Jan 9, 2020 via email

straight-shoota commented Jan 9, 2020

RX14 Jan 10, 2020

RX14 commented Jan 10, 2020

straight-shoota commented Feb 18, 2020

jan-zajic commented Feb 26, 2020

oprypin commented Apr 3, 2020

oprypin commented Apr 3, 2020

oprypin commented Apr 3, 2020

jan-zajic commented Apr 8, 2020

jan-zajic commented Apr 8, 2020

Rename Process kill methods to signal #8642

Rename Process kill methods to signal #8642

Conversation

jan-zajic commented Jan 2, 2020

bcardiff commented Jan 2, 2020

Blacksmoke16 commented Jan 2, 2020 • edited Loading

jan-zajic commented Jan 2, 2020

jan-zajic commented Jan 2, 2020

bcardiff commented Jan 2, 2020

jan-zajic commented Jan 3, 2020

straight-shoota left a comment

Choose a reason for hiding this comment

straight-shoota Jan 6, 2020

Choose a reason for hiding this comment

straight-shoota Jan 6, 2020

Choose a reason for hiding this comment

jan-zajic commented Jan 8, 2020 • edited Loading

straight-shoota commented Jan 8, 2020

Sija commented Jan 8, 2020

j8r commented Jan 8, 2020

RX14 commented Jan 8, 2020 • edited Loading

Sija commented Jan 8, 2020 • edited Loading

straight-shoota commented Jan 8, 2020

RX14 commented Jan 8, 2020

RX14 commented Jan 8, 2020

Sija commented Jan 8, 2020 • edited Loading

straight-shoota commented Jan 8, 2020 • edited Loading

RX14 commented Jan 9, 2020

RX14 commented Jan 9, 2020

jan-zajic commented Jan 9, 2020 via email

straight-shoota commented Jan 9, 2020

RX14 Jan 10, 2020

Choose a reason for hiding this comment

RX14 commented Jan 10, 2020

straight-shoota commented Feb 18, 2020

jan-zajic commented Feb 26, 2020

oprypin commented Apr 3, 2020

oprypin commented Apr 3, 2020

oprypin commented Apr 3, 2020

jan-zajic commented Apr 8, 2020

jan-zajic commented Apr 8, 2020

Blacksmoke16 commented Jan 2, 2020 •

edited

Loading

jan-zajic commented Jan 8, 2020 •

edited

Loading

RX14 commented Jan 8, 2020 •

edited

Loading

Sija commented Jan 8, 2020 •

edited

Loading

Sija commented Jan 8, 2020 •

edited

Loading

straight-shoota commented Jan 8, 2020 •

edited

Loading