New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add crossplatform kill method #3499

Merged
merged 7 commits into from Mar 1, 2018

Conversation

Projects
None yet
4 participants
@adcxyz
Copy link
Contributor

adcxyz commented Feb 6, 2018

this PR adds a crossplatform kill method to allow killing specific process ids (in addition to killAll).
this is useful for ending specific cmdline programs from within SC. E.g. in a current UnitTest where a server program is started by hand, #3486, this is useful to make that test run crossplatform.
not tested on windows yet - windows devs please check :-)

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 6, 2018

'kill' is very general, I think it ought to be something like 'killProcessByID'

@adcxyz

This comment has been minimized.

Copy link
Contributor

adcxyz commented Feb 6, 2018

ok done

@@ -172,6 +172,10 @@ Platform {
// hook for clients to write frontend.css
writeClientCSS {}

killProcessByID { |cmdLineArgs|
^this.subclassResponsibility(\kill)

This comment has been minimized.

@snappizz

snappizz Feb 6, 2018

Member

this should be ^this.subclassResponsibility(\killProcessByID), no?

This comment has been minimized.

@adcxyz

adcxyz Feb 6, 2018

Contributor

yes right, thanks for spotting it!

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 6, 2018

testing this out on windows now :)

@@ -172,6 +172,10 @@ Platform {
// hook for clients to write frontend.css
writeClientCSS {}

killProcessByID { |cmdLineArgs|

This comment has been minimized.

@snappizz

snappizz Feb 6, 2018

Member

this argument should be pid

This comment has been minimized.

@adcxyz

adcxyz Feb 9, 2018

Contributor

ok done.

@snappizz

This comment has been minimized.

Copy link
Member

snappizz commented Feb 6, 2018

i think this method is a good idea, but nominally our policy is that we don't introduce new features during 3.9.x. i'm not fully decided on whether to bend that policy for this case.

if we do decide to commit to this as a new feature, i'd like to see unit tests and documentation. otherwise it's also fine to just make this a private method for now and decide later to make it public.

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 6, 2018

I think nathan is right, I was a little too ambitious here in thinking this could go into a patch release. i definitely want it as a feature because it seems genuinely useful to have a cross-platform way to do this.

if we do decide to commit to this as a new feature, i'd like to see unit tests and documentation.

i agree, i would be happy to add documentation for this and for killAll to this PR. what would the unit tests for this look like?

otherwise it's also fine to just make this a private method for now and decide later to make it public.

I'd be OK with that (although it seems like extra work for such a specific case). For #3486, maybe the best solution for now is to just stick the code directly into the test, without encapsulating it in a method, but leave a TODO that it can be updated in 3.10

@snappizz

This comment has been minimized.

Copy link
Member

snappizz commented Feb 6, 2018

on second thought we might not need unit tests in this case, i think it's pretty transparent. docs at the very least.

@adcxyz

This comment has been minimized.

Copy link
Contributor

adcxyz commented Feb 8, 2018

@brianlheim @snappizz - how do you want to continue with this and #3486 ?
To me, this looks a bit like buraucratic gridlock ;-) :

  • user reports rare but potentially show-stopping bug with remote server logins
  • PR #3486 fixes that and for completeness, adds unit tests for topic remote & repeated logins
  • unit tests require a crossplatform utility method for killing a single process by pid
  • should crossplatform utility method be added to next patch release or not (for policy reasons)?

I understand that 'no new features in patch releases' is meant to secure stability.
In this case, I don't see any potential instability (or other downsides) introduced adding killProcessByID now, and the advantage would be that both killProcessByID and the remote login tests are done properly now, leaving no FIXMEs for later.

But it is your call, so just let me know.
If you prefer the temp solution for 3.9.2, would that be something like this?

TestServer_clientID_booted { 
...
	// temp replace Platform.killProcessByID
	prKillProcessByID { |pid|
		Platform.case(
			\windows, { ("taskkill /F /pid " ++ pid).unixCmd }, 
			{ ("kill -9 " ++ pid).unixCmd }
		)
	}
	test_remoteLogin {
		var options, serverPid, remote1, cond, timer;
		...
		remote1.remove;
		// FIXME: when addKillMethod PR is in, replace with
		// thisProcess.platform.killProcessByID(serverPid);
		this.prKillProcessByID(serverPid);
	}

	test_repeatedRemoteLogin {
		...
		// cleanup
		remote1.remove;
		remote2.remove;
		// FIXME: when addKillMethod PR is in, replace with
		// thisProcess.platform.killProcessByID(serverPid);
		this.prKillProcessByID(serverPid);
	}
@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 8, 2018

If you prefer the temp solution for 3.9.2, would that be something like this?

Yes, but the method doesn't have to be named pr if it's on a test class.

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Feb 8, 2018

We shouldn't forbid refactorings in minor releases, I think. This could enforce a bad programming style. But refactorings usually introduce new methods, which I wouldn't call features.

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Feb 8, 2018

… what I would try to avoid in minor releases would be methods that are exposed to a larger combinatorics of potential uses.

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 8, 2018

I think it's better to follow the practices we agreed on. These can be found in the git flow and guidelines page of the wiki.

The other goal of having this rule is that it saves time that can be better spent on more important tasks. For example, we have already spend more time here than it would take to just wait until 3.10 and swap out the method. If we repeat that for every case like this, it becomes very un fun. I'm sorry for the confusion, it's totally my fault because I recommended a solution that wasn't totally correct according to our process.

I don't mean this as an argument for "let's follow rules for the sake of it". I just want some sort of stability and agreement so we can stop splitting hairs. If we never establish any firm rule whatsoever, there's no chance of that.

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 8, 2018

Also, meta discussions like this are probably better had on the mailing list or a separate issue ticket.

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Feb 8, 2018

I don't mean this as an argument for "let's follow rules for the sake of it". I just want some sort of stability and agreement so we can stop splitting hairs. If we never establish any firm rule whatsoever, there's no chance of that.

sure, I'm fine with rules. I've tried to find the rule you are alluding to, but rereading the guidelines, I couldn't find it. It's a lot of stuff, maybe I've overlooked it.

For example, we have already spend more time here than it would take to just wait until 3.10 and swap out the method.

We need to keep the flow. 3.10 is scheduled for early July, and I wouldn't want to wait so long. But let's move the discussion to the mailing list. (done just now.)

@adcxyz

This comment has been minimized.

Copy link
Contributor

adcxyz commented Feb 9, 2018

Ok, thanks for your understanding, and sorry for getting on your nerves with this.
To finish this one, I just did the last requested change.
To make it an official new feature in the next 3.x release, we could then (not now)
add class redirects to killAll and killProcessByID, so one can then write :

Platform.killProcessByID(1234);
Platform.killAll("scsynth");
@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 9, 2018

OK, thanks! Still needs documentation.

@adcxyz

This comment has been minimized.

Copy link
Contributor

adcxyz commented Feb 10, 2018

ok, doc added!

@brianlheim
Copy link
Member

brianlheim left a comment

I can confirm that this works on Windows, but the documentation could be a little more conforming.

@@ -144,6 +144,25 @@ files to be loaded on startup
method:: loadStartupFiles
(re)load startup files

subsection:: Crossplatform system commands

This comment has been minimized.

@brianlheim

brianlheim Feb 19, 2018

Member

"Crossplatform" is redundant here

This comment has been minimized.

@adcxyz

adcxyz Feb 22, 2018

Contributor

yes


method:: killAll
kill all processes as defined by cmdLineArgs.
Typically, this is a process name or a list of names.

This comment has been minimized.

@brianlheim

brianlheim Feb 19, 2018

Member

these descriptions should use the argument:: tag, and specify the expected type of the argument

This comment has been minimized.

@adcxyz

adcxyz Feb 22, 2018

Contributor

ok

::

method:: killProcessByID
kill all processes as identified by pid argument.

This comment has been minimized.

@brianlheim

brianlheim Feb 19, 2018

Member

PIDs uniquely identify a process, so saying "kill all" doesn't make sense here.

This comment has been minimized.

@adcxyz

adcxyz Feb 22, 2018

Contributor

yes

@adcxyz

This comment has been minimized.

Copy link
Contributor

adcxyz commented Feb 22, 2018

@brianlheim thanks for the precise comments, all done now.

@brianlheim
Copy link
Member

brianlheim left a comment

Thanks!

Resolved by @adcxyz's changes

@brianlheim brianlheim merged commit 62a3f0d into supercollider:3.9 Mar 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adcxyz adcxyz deleted the adcxyz:addKillMethod branch Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment