Skip to content
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

auto split fix #495

Merged
merged 4 commits into from
Mar 14, 2022
Merged

auto split fix #495

merged 4 commits into from
Mar 14, 2022

Conversation

tammam1998
Copy link
Collaborator

@tammam1998 tammam1998 commented Mar 12, 2022

The hanging issue seems to be resolved if we open all output files at the beginning instead of one by one. From my investigation, I believe that this happens because when tr exists, auto split receives SIGPIPE and exits before opening any of the remaining output files causing other processes to hang. Another solution that seems to work is adding signal(SIGPIPE, SIG_IGN); to ignore sigpipe errors and then handle it (open and close remaining fifos) by checking fifo errors.

@angelhof both solutions seem to work, this pr contains the first one but if you think the second solution is better let me know and I can push it.

Signed-off-by: Tammam Mustafa <[email protected]>
@tammam1998 tammam1998 linked an issue Mar 12, 2022 that may be closed by this pull request
@github-actions
Copy link

OS:ubuntu-18.04
Sat Mar 12 17:53:36 UTC 2022
intro: 2/2 tests passed.
interface: 34/34 tests passed.
compiler: 54/54 tests passed.
agg: 111/126 tests passed.
test-22 are not identical
test-23 are not identical
test-33 are not identical
test-60 are not identical
test-124 are not identical
test-127 are not identical
test-128 are not identical
test-154 are not identical
test-156 are not identical
test-157 are not identical
test-159 are not identical
test-161 are not identical
test-162 are not identical
test-186 are not identical
test-191 are not identical

@github-actions
Copy link

OS:ubuntu-18.04
Sat Mar 12 17:54:04 UTC 2022
intro: 2/2 tests passed.
interface: 34/34 tests passed.
compiler: 54/54 tests passed.
agg: 111/126 tests passed.
test-22 are not identical
test-23 are not identical
test-33 are not identical
test-60 are not identical
test-124 are not identical
test-127 are not identical
test-128 are not identical
test-154 are not identical
test-156 are not identical
test-157 are not identical
test-159 are not identical
test-161 are not identical
test-162 are not identical
test-186 are not identical
test-191 are not identical

@angelhof
Copy link
Member

This solution looks good to me! @dkarnikis can you check which tests of the new agg tests are supposed to pass and which are supposed to fail so that we didn't introduce any regression? In general, let's disable the failing agg tests until we make them work so that it is clear when merging PRs that we haven't introduced regressions.

Feel free to merge this after we ensure no regressions

Signed-off-by: Dimitris Karnikis <[email protected]>
@dkarnikis
Copy link
Collaborator

dkarnikis commented Mar 14, 2022

@tammam1998 When running the fix-test-191 (this is the same as the issue I created), your fix indeed prevents hanging, however i get the following output:

tr: when not truncating set1, string2 must be non-empty
tr: when not truncating set1, string2 must be non-empty

or

tr: tr: when not truncating set1, string2 must be non-emptywhen not truncating set1, string2 must be non-empty

Whereas bash reports: tr: when not truncating set1, string2 must be non-empty

The return code for pash is 0 whereas on bash it's 1

@github-actions
Copy link

OS:ubuntu-20.04
Mon Mar 14 09:42:42 UTC 2022
intro: 2/2 tests passed.
interface: 34/34 tests passed.
compiler: 54/54 tests passed.
agg: 111/111 tests passed.

@github-actions
Copy link

OS:ubuntu-18.04
Mon Mar 14 09:44:15 UTC 2022
intro: 2/2 tests passed.
interface: 34/34 tests passed.
compiler: 54/54 tests passed.
agg: 111/111 tests passed.

Signed-off-by: Dimitris Karnikis <[email protected]>
@github-actions
Copy link

OS:ubuntu-18.04
Mon Mar 14 10:14:38 UTC 2022
intro: 2/2 tests passed.
interface: 34/34 tests passed.
compiler: 54/54 tests passed.
agg: 111/111 tests passed.

@github-actions
Copy link

OS:ubuntu-20.04
Mon Mar 14 10:14:43 UTC 2022
intro: 2/2 tests passed.
interface: 34/34 tests passed.
compiler: 54/54 tests passed.
agg: 111/111 tests passed.

@angelhof
Copy link
Member

@tammam1998 When running the fix-test-191 (this is the same as the issue I created), your fix indeed prevents hanging, however i get the following output:

tr: when not truncating set1, string2 must be non-empty
tr: when not truncating set1, string2 must be non-empty

or

tr: tr: when not truncating set1, string2 must be non-emptywhen not truncating set1, string2 must be non-empty

Whereas bash reports: tr: when not truncating set1, string2 must be non-empty

The return code for pash is 0 whereas on bash it's 1

This is probably the stderror output (which we never capture or compare with PaSh and it is OK if it is duplicated etc). Furthermore, at the moment I don't think we would return this exit code correctly. We could open an issue for someone to take a look at this and try to figure out a way to return the expected exit code in such cases.

@dkarnikis dkarnikis merged commit 0104aa9 into future Mar 14, 2022
@dkarnikis dkarnikis deleted the auto-split-hang-fix branch March 14, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tr hanging issue
3 participants