Skip to content

timeout: fix subprocess is never terminated#4315

Merged
tertsdiepraam merged 2 commits intouutils:mainfrom
miles170:issue-4176-fix-timeout
Mar 15, 2023
Merged

timeout: fix subprocess is never terminated#4315
tertsdiepraam merged 2 commits intouutils:mainfrom
miles170:issue-4176-fix-timeout

Conversation

@miles170
Copy link
Copy Markdown
Contributor

@miles170 miles170 commented Feb 1, 2023 •
edited
Loading

Closes #4176.

This should also fix #4073.

Follow GNU's timeout behavior, send a signal to the process group to make sure subprocesses are cleaned up, and also send SIGCONT to the child and the whole process group after that.

@sylvestre
Copy link
Copy Markdown
Contributor

sylvestre commented Feb 2, 2023

would it be possible to add a test for this?
Thanks

@miles170 miles170 marked this pull request as draft February 3, 2023 01:03
@miles170 miles170 force-pushed the issue-4176-fix-timeout branch 2 times, most recently from f490aaf to 7f03e29 Compare February 3, 2023 03:35
@miles170 miles170 marked this pull request as ready for review February 3, 2023 03:35
@miles170
Copy link
Copy Markdown
Contributor Author

miles170 commented Feb 3, 2023 •
edited
Loading

would it be possible to add a test for this? Thanks

Added test_kill_subprocess test.

@miles170 miles170 marked this pull request as draft February 3, 2023 04:00
@miles170 miles170 force-pushed the issue-4176-fix-timeout branch 2 times, most recently from 49fc1ac to edd81ed Compare February 3, 2023 05:50
@miles170 miles170 marked this pull request as ready for review February 3, 2023 05:57
@miles170 miles170 force-pushed the issue-4176-fix-timeout branch 2 times, most recently from 2ac1cb7 to 5dadb70 Compare February 3, 2023 06:30
@sylvestre sylvestre force-pushed the issue-4176-fix-timeout branch from 5dadb70 to 6fade5a Compare February 7, 2023 08:22
@miles170 miles170 force-pushed the issue-4176-fix-timeout branch from 6fade5a to bf2b5f5 Compare February 8, 2023 02:15
@miles170
Copy link
Copy Markdown
Contributor Author

miles170 commented Feb 8, 2023

I'm not sure why the CI is failing randomly.
I have increased the timeout from 2 seconds to 5 seconds. (Everything works fine on my local Ubuntu 22.10)

@miles170 miles170 force-pushed the issue-4176-fix-timeout branch from bf2b5f5 to 6f42d5c Compare February 8, 2023 03:14
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 8, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@miles170 miles170 force-pushed the issue-4176-fix-timeout branch from 6f42d5c to b9d3c48 Compare February 8, 2023 03:18
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 8, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@miles170 miles170 force-pushed the issue-4176-fix-timeout branch from b9d3c48 to 3678c6e Compare February 8, 2023 06:26
@miles170
Copy link
Copy Markdown
Contributor Author

miles170 commented Feb 8, 2023

I have increased the timeout from 5 seconds to 10 seconds, and the failing tests are not related to this PR.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 8, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@sylvestre sylvestre force-pushed the issue-4176-fix-timeout branch from 3678c6e to 46e94f1 Compare March 5, 2023 20:38
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 5, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@miles170 miles170 force-pushed the issue-4176-fix-timeout branch from 46e94f1 to f406b56 Compare March 10, 2023 10:09
tertsdiepraam
tertsdiepraam approved these changes Mar 12, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reran the CI because of some unrelated linting issues that should be fixed by now, but this PR looks good.

@miles170
Copy link
Copy Markdown
Contributor Author

miles170 commented Mar 15, 2023 •
edited
Loading

I checked the GNU test logs and found that this should also fix #4073, which is caused by the split subprocess (filter) not being killed by the timeout.

@tertsdiepraam
Copy link
Copy Markdown
Collaborator

tertsdiepraam commented Mar 15, 2023

Indeed this reduces the log size from 86MB to just 2MB, that's great! Let's hope the CI becomes a bit more stable as a result.

@tertsdiepraam tertsdiepraam merged commit dad474b into uutils:main Mar 15, 2023
@tertsdiepraam tertsdiepraam mentioned this pull request Mar 15, 2023
Closed
@miles170 miles170 deleted the issue-4176-fix-timeout branch March 20, 2023 08:16
@tertsdiepraam tertsdiepraam mentioned this pull request Sep 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tertsdiepraam tertsdiepraam tertsdiepraam approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

timeout: subprocess is never terminated split produces one million lines of error messages in the GNU test suite

3 participants

@miles170 @sylvestre @tertsdiepraam