openSUSE / Mojo-IOLoop-ReadWriteProcess

Execute external programs or internal code blocks as separate process

Other

10 stars 13 forks source link

Improve stopping #9

Closed Martchus closed 4 years ago

Martchus commented 4 years ago

See particular commit messages
See https://progress.opensuse.org/issues/67342 for the full story
- TL;DR: We want to allow ffmpeg to finalize its video.
  - For that we need to prevent spamming SIGTERM signals on it without introducing long sleep times.
  - We also need to be able to stop the full process group and wait for the termination of the full process group to know when ffmpeg has finalized the video.

Martchus commented 4 years ago

I would also like to note that I haven't had that much time yesterday to test it. So I'll better add a WIP prefix.

Martchus commented 4 years ago

I was on vacation and now the CI tests on openQA are broken so I haven't had the time to look into this PR further (but I have not abandoned it). The suggestions so far make sense and I'll of course look into the failing tests.

codecov-commenter commented 4 years ago

Codecov Report

Merging #9 into master will increase coverage by 0.26%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
+ Coverage   88.76%   89.02%   +0.26%     
==========================================
  Files          27       27              
  Lines         881      893      +12     
  Branches      200      202       +2     
==========================================
+ Hits          782      795      +13     
  Misses         21       21              
+ Partials       78       77       -1

Impacted Files	Coverage Δ
lib/Mojo/IOLoop/ReadWriteProcess.pm	`90.72% <100.00%> (+0.75%)`	: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 22ddac0...54b3db8. Read the comment docs.

Martchus commented 4 years ago

I've now updated the code and added/adapted unit tests. It seems to work now. I guess the failing tests for Perl < 5.16 can be ignored?

By the way, I've also tested this by running and stopping an openQA job using ffmpeg as videoencoder. Everything seems to work (no delays, no truncated video including the uploaded video).

foursixnine commented 4 years ago

@Martchus Can you rebase?

Martchus commented 4 years ago

Rebased. By the way, it would be nice to make a new release once this change has been merged so we can make use of it in openQA without making a Git package.

Martchus commented 4 years ago

I've also realized that with the latest version of tidy there's a huge diff. So maybe before the release I can create yet another PR to simply apply the latest version of tidy.

(The changes here are already tidied with the latest version of tidy but I didn't include unrelated tidying.)

foursixnine commented 4 years ago

I've also realized that with the latest version of tidy there's a huge diff. So maybe before the release I can create yet another PR to simply apply the latest version of tidy.

(The changes here are already tidied with the latest version of tidy but I didn't include unrelated tidying.)

@mudler usually does the releases, so pre or post tidy would be more up to him, but sending the PR might make sense