Skip to content

tee: do not retry on EINTR#12505

Open
gabrielhnf wants to merge 4 commits into
uutils:mainfrom
gabrielhnf:fix-tee-eintr-unconditional-retry
Open

tee: do not retry on EINTR#12505
gabrielhnf wants to merge 4 commits into
uutils:mainfrom
gabrielhnf:fix-tee-eintr-unconditional-retry

Conversation

@gabrielhnf
Copy link
Copy Markdown
Contributor

Related to #12469

Writer::write_all was delegating to the standard library's write_all, which unconditionally retries on EINTR.

Replaces the write_all delegation with a manual loop that calls write() directly and propagates EINTR immediately without retrying.

@gabrielhnf gabrielhnf force-pushed the fix-tee-eintr-unconditional-retry branch from b35b6a7 to b78b745 Compare May 28, 2026 13:09
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 28, 2026

Merging this PR will not alter performance

✅ 319 untouched benchmarks
⏩ 46 skipped benchmarks1


Comparing gabrielhnf:fix-tee-eintr-unconditional-retry (85a6062) with main (55549fa)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@oech3

This comment was marked as duplicate.

Comment thread src/uu/tee/src/tee.rs Outdated
s.write_all(buf)?;
// needs unsafe to remove buffering... flush after write_all to keep overhead minimal
s.flush()
let mut buf = buf;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it needs comment

@oech3

This comment was marked as resolved.

@gabrielhnf
Copy link
Copy Markdown
Contributor Author

How about adding write_all_with_eintr to https://github.com/uutils/coreutils/blob/main/src/uucore/src/lib/mods/io.rs instead? This PR is duplicating code.

Yeah, might be a good idea. Will close the PR and research the issue

@gabrielhnf gabrielhnf closed this May 28, 2026
@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 28, 2026

Actually, I was considering to dedup some code with cat's manual loop. cat were not affected.

@gabrielhnf
Copy link
Copy Markdown
Contributor Author

Are you planning to tackle this yourself then?

@oech3

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 28, 2026

GNU testsuite comparison:

Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)

@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 28, 2026

I think there is no compatibility requirement for non-unix. But does EINTR exist on Windows?

@gabrielhnf gabrielhnf reopened this May 28, 2026
@oech3

This comment was marked as resolved.

@gabrielhnf gabrielhnf force-pushed the fix-tee-eintr-unconditional-retry branch from 223039b to 44a6430 Compare May 28, 2026 14:39
@gabrielhnf
Copy link
Copy Markdown
Contributor Author

i did not add write_all_no_retry for non-unix...

Sorry, got things a little messy here and accidentally changed that. Just fixed it

@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 28, 2026

Thanks. It is my mistake.

@gabrielhnf gabrielhnf force-pushed the fix-tee-eintr-unconditional-retry branch 2 times, most recently from 1de19a6 to 85a6062 Compare May 28, 2026 15:01
@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 29, 2026

I noticed that there is

  -i, --ignore-interrupts
          ignore interrupt signals (ignored on non-Unix platforms)

. So this solution is still incomplete. done at different place?

@gabrielhnf
Copy link
Copy Markdown
Contributor Author

Doesn't seem to be an issue, -i​ sets SIGINT​ to SIG_IGN​ through ignore_interrupts()​ at the start of tee()​, so write()​ shouldn't get EINTR​ from it, no?

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.

3 participants