Re: [Avocado-devel] Subprocess termination
Hi. It seems to me, that you are talking about global task: "track of special resources" Radek's case is much simple: kill all children. I think it is correct behavior. This is as it should be. There is nothing to track. On opposite side, if test want to keep running process that it can detach it and make it daemon. On Fri, Feb 17, 2017 at 6:11 PM, Lucas Meneghel Rodrigueswrote: > I would avoid keeping track of special resources by the test runner > itself. It's the sort of thing that the test writer would be expected to > implement on cleanup procedures. > > Implementing said track of subprocesses would be justifiable if we look at > the following perspectives: > > 1) We want to reduce the amount of boilerplate code in tests. This would > hold true if Radek's use case was very common, like in many occasions you > want to spawn a process in the background and forget about it. It doesn't > seem it is. > 2) We want avocado to be very good in avoid leaking resources regardless > of decisions made by test writers. > > I can see the point 2) as valid, but again, it's introducing a lot of code > to handle certain resources transparently for users that might not need > that on a regular basis. > > On Fri, Feb 17, 2017 at 11:55 AM Amador Pahim wrote: > >> On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda wrote: >> > Good morning folks, >> > Andrei thanks for this line of code I've forgotten to include it. So >> the >> > code reads: >> > >> > def run(vt_test, test_params, env): >> > cmd = "nc -l %s" % test_params['some_port'] >> > nc_process = process.SubProcess(cmd) >> > nc_process_pid = nc_process.start() >> > return >> > >> > Amador thanks for your advice, but it doesn't work for me. I need the >> > process to run in the background and not block test flow. That's why I >> used >> > start() method. I tried run() method as well, but the process does not >> go to >> > background and blocked test flow. I think the process should be killed >> > automatically after test finishes. >> >> Oh, now I see what you need. >> Well, I'd recommend to finish the process in tearDown(). Not sure if >> we should make the runner to track the sub-processes created by the >> test... let me open a card to discuss that. >> >> >> > >> > Radek >> > >> > On Thu, Feb 16, 2017 at 5:09 PM, Amador Pahim >> wrote: >> >> >> >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov >> >> wrote: >> >> > I think >> >> > >> >> > nc_process_pid = nc_process.start() >> >> >> >> In that case, the missing part is the `wait()` (and the possible >> >> timeout handling) which are both present in `run()`. You can call it >> >> directly (with `process.run(cmd)`) instead of keeping the SubProcess >> >> object. Unless you have other reasons to have that object. >> >> >> >> > >> >> > On Thu, Feb 16, 2017 at 4:58 PM, Amador Pahim >> wrote: >> >> >> >> >> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda >> wrote: >> >> >> > Dear avocado users and developers, >> >> >> > I have made a testcase in which is executed nc process in run >> >> >> > function : >> >> >> > (simplified version): >> >> >> > >> >> >> > from avocado.utils import process >> >> >> > >> >> >> > def run(vt_test, test_params, env): >> >> >> > cmd = "nc -l %s" % test_params['some_port'] >> >> >> > nc_process = process.SubProcess(cmd) >> >> >> > return >> >> >> > >> >> >> > after testcase is executed, nc does not terminate and is still >> >> >> > present. >> >> >> > To >> >> >> > avoid this I have to kill the process e.g. by >> >> >> > process.safe_kill(nc_process_pid, signal.SIGKILL) >> >> >> >> >> >> Are you running the process with `nc_process.run()`? It's expected >> to >> >> >> wait for the process to finish. >> >> >> >> >> >> >> >> >> >> >> >> https://github.com/avocado-framework/avocado/blob/master/ >> avocado/utils/process.py#L596-L643 >> >> >> >> >> >> >> >> >> > >> >> >> > It is pretty awkward to close the process manually particularly in >> >> >> > case >> >> >> > of >> >> >> > complex testcase code >> >> >> > Shouldn't be the subprocess killed automatically after test exits? >> >> >> > After all its called SubProcess >> >> >> > >> >> >> > >> >> >> > regards, >> >> >> > >> >> >> > Radek Duda >> >> >> >> >> > >> > >> > >> >>
Re: [Avocado-devel] Subprocess termination
I would avoid keeping track of special resources by the test runner itself. It's the sort of thing that the test writer would be expected to implement on cleanup procedures. Implementing said track of subprocesses would be justifiable if we look at the following perspectives: 1) We want to reduce the amount of boilerplate code in tests. This would hold true if Radek's use case was very common, like in many occasions you want to spawn a process in the background and forget about it. It doesn't seem it is. 2) We want avocado to be very good in avoid leaking resources regardless of decisions made by test writers. I can see the point 2) as valid, but again, it's introducing a lot of code to handle certain resources transparently for users that might not need that on a regular basis. On Fri, Feb 17, 2017 at 11:55 AM Amador Pahimwrote: > On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda wrote: > > Good morning folks, > > Andrei thanks for this line of code I've forgotten to include it. So the > > code reads: > > > > def run(vt_test, test_params, env): > > cmd = "nc -l %s" % test_params['some_port'] > > nc_process = process.SubProcess(cmd) > > nc_process_pid = nc_process.start() > > return > > > > Amador thanks for your advice, but it doesn't work for me. I need the > > process to run in the background and not block test flow. That's why I > used > > start() method. I tried run() method as well, but the process does not > go to > > background and blocked test flow. I think the process should be killed > > automatically after test finishes. > > Oh, now I see what you need. > Well, I'd recommend to finish the process in tearDown(). Not sure if > we should make the runner to track the sub-processes created by the > test... let me open a card to discuss that. > > > > > > Radek > > > > On Thu, Feb 16, 2017 at 5:09 PM, Amador Pahim wrote: > >> > >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov > >> wrote: > >> > I think > >> > > >> > nc_process_pid = nc_process.start() > >> > >> In that case, the missing part is the `wait()` (and the possible > >> timeout handling) which are both present in `run()`. You can call it > >> directly (with `process.run(cmd)`) instead of keeping the SubProcess > >> object. Unless you have other reasons to have that object. > >> > >> > > >> > On Thu, Feb 16, 2017 at 4:58 PM, Amador Pahim > wrote: > >> >> > >> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda > wrote: > >> >> > Dear avocado users and developers, > >> >> > I have made a testcase in which is executed nc process in run > >> >> > function : > >> >> > (simplified version): > >> >> > > >> >> > from avocado.utils import process > >> >> > > >> >> > def run(vt_test, test_params, env): > >> >> > cmd = "nc -l %s" % test_params['some_port'] > >> >> > nc_process = process.SubProcess(cmd) > >> >> > return > >> >> > > >> >> > after testcase is executed, nc does not terminate and is still > >> >> > present. > >> >> > To > >> >> > avoid this I have to kill the process e.g. by > >> >> > process.safe_kill(nc_process_pid, signal.SIGKILL) > >> >> > >> >> Are you running the process with `nc_process.run()`? It's expected to > >> >> wait for the process to finish. > >> >> > >> >> > >> >> > >> >> > https://github.com/avocado-framework/avocado/blob/master/avocado/utils/process.py#L596-L643 > >> >> > >> >> > >> >> > > >> >> > It is pretty awkward to close the process manually particularly in > >> >> > case > >> >> > of > >> >> > complex testcase code > >> >> > Shouldn't be the subprocess killed automatically after test exits? > >> >> > After all its called SubProcess > >> >> > > >> >> > > >> >> > regards, > >> >> > > >> >> > Radek Duda > >> >> > >> > > > > > > >
Re: [Avocado-devel] Subprocess termination
Good morning folks, Andrei thanks for this line of code I've forgotten to include it. So the code reads: def run(vt_test, test_params, env): cmd = "nc -l %s" % test_params['some_port'] nc_process = process.SubProcess(cmd) nc_process_pid = nc_process.start() return Amador thanks for your advice, but it doesn't work for me. I need the process to run in the background and not block test flow. That's why I used start() method. I tried run() method as well, but the process does not go to background and blocked test flow. I think the process should be killed automatically after test finishes. Radek On Thu, Feb 16, 2017 at 5:09 PM, Amador Pahimwrote: > On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov > wrote: > > I think > > > > nc_process_pid = nc_process.start() > > In that case, the missing part is the `wait()` (and the possible > timeout handling) which are both present in `run()`. You can call it > directly (with `process.run(cmd)`) instead of keeping the SubProcess > object. Unless you have other reasons to have that object. > > > > > On Thu, Feb 16, 2017 at 4:58 PM, Amador Pahim wrote: > >> > >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda wrote: > >> > Dear avocado users and developers, > >> > I have made a testcase in which is executed nc process in run > function : > >> > (simplified version): > >> > > >> > from avocado.utils import process > >> > > >> > def run(vt_test, test_params, env): > >> > cmd = "nc -l %s" % test_params['some_port'] > >> > nc_process = process.SubProcess(cmd) > >> > return > >> > > >> > after testcase is executed, nc does not terminate and is still > present. > >> > To > >> > avoid this I have to kill the process e.g. by > >> > process.safe_kill(nc_process_pid, signal.SIGKILL) > >> > >> Are you running the process with `nc_process.run()`? It's expected to > >> wait for the process to finish. > >> > >> > >> https://github.com/avocado-framework/avocado/blob/master/ > avocado/utils/process.py#L596-L643 > >> > >> > >> > > >> > It is pretty awkward to close the process manually particularly in > case > >> > of > >> > complex testcase code > >> > Shouldn't be the subprocess killed automatically after test exits? > >> > After all its called SubProcess > >> > > >> > > >> > regards, > >> > > >> > Radek Duda > >> > > >