openSUSE / Mojo-IOLoop-ReadWriteProcess

Execute external programs or internal code blocks as separate process

Other

10 stars 13 forks source link

In "stop" sleep only after sending the first signal to speedup termination #6

Closed okurz closed 4 years ago

okurz commented 4 years ago

By default the sleep time between sending term/kill signals is 1s meaning that previously the termination of each process took at least 1s which adds up in multi-process applications. There should be no need to wait until we send out the first signal to the process to stop.

foursixnine commented 4 years ago

@okurz the tests in t/01_run.t are failing

okurz commented 4 years ago

yes, I can see that. I have not yet understood if this is due to my changes or if it might be a problem on current master already. Regardless of tests failing I would be happy to hear thoughts about the idea in general if I am on the right track :)

foursixnine commented 4 years ago

Current master seems to be passing(https://travis-ci.org/mudler/Mojo-IOLoop-ReadWriteProcess/builds), but moving the sleep is on the right track :). After all, you kill processes and wait, not the other way around

okurz commented 4 years ago

I crosschecked the master in my fork and it is all green: https://travis-ci.org/okurz/Mojo-IOLoop-ReadWriteProcess/builds/618218983 so my changes seem to really cause this :)

okurz commented 4 years ago

I think I understood now what happens. The one subtest failing (reproduced in all travis CI test jobs the same) was relying on the stop() call waiting long enough internally so that the spawned process in the meantime would be able to write output to STDERR. The subtest calls "stop()" immediately after "start()". Other subtests use a different approach. The most simple solution I could find is to apply the same as the subtest in before, i.e. a sleep 1. I don't like to put sleeps in code but this isn't really my code and as the design seem to be around absolute waiting time I don't want to introduce further change in this step. Updated the PR.

codecov-io commented 4 years ago

Codecov Report

Merging #6 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##           master       #6   +/-   ##
=======================================
  Coverage   88.92%   88.92%           
=======================================
  Files          27       27           
  Lines         885      885           
  Branches      203      203           
=======================================
  Hits          787      787           
  Misses         20       20           
  Partials       78       78

Impacted Files	Coverage Δ
lib/Mojo/IOLoop/ReadWriteProcess.pm	`90.68% <100%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 62e3947...fb9bc63. Read the comment docs.

foursixnine commented 4 years ago

The most simple solution I could find is to apply the same as the subtest in before, i.e. a sleep 1

Simple is good ;)

okurz commented 4 years ago

thanks for merge. Now I see builds in master failing though, e.g. https://travis-ci.org/github/mudler/Mojo-IOLoop-ReadWriteProcess/jobs/662961422 . will look into this.