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

Handle EINTR. #20

Merged
merged 1 commit into from
Jan 13, 2020
Merged

Handle EINTR. #20

merged 1 commit into from
Jan 13, 2020

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jan 12, 2020

When building with sccache locally, I'm seeing a bunch of errors to acquire the
jobserver token which are caused by interrupts at bad times.

I think it was a kernel update what caused this, fwiw, but not 100% sure.

Both can happen according to the man pages, and we're not handling them
sensibly.

I think retrying the read / poll when the error is EINTR is a sensible thing to
do.

@emilio
Copy link
Contributor Author

emilio commented Jan 12, 2020

With this patch so far I'm able to build Firefox with sccache-dist on a row. Before this, I've needed always a couple restarts per build to deal with this.

This may have something to do with the pipe changes / make bug described in here: https://lkml.org/lkml/2019/12/7/173

@alexcrichton
Copy link
Member

Thanks! Instead of introducing nested loops could these continue to the outer loop?

@emilio
Copy link
Contributor Author

emilio commented Jan 13, 2020

Thanks! Instead of introducing nested loops could these continue to the outer loop?

I don't think that's equivalent right? If the read gets interrupted we want to re-trigger the read, not the poll.

@emilio
Copy link
Contributor Author

emilio commented Jan 13, 2020

(FWIW, I've been building with a patched sccache for the whole day and had zero issues)

@alexcrichton
Copy link
Member

While not literally equivalent it's morally equivalent, I don't mean to contradict the correctness of this patch! Only that I think it would be cleaner to only have one loop and we continue to that loop. While it's technically more optimal to avoid a poll after the read has been interrupted I don't think it really matters in terms of perf, so I would prefer to err on the side of more readable style.

When building with sccache locally, I'm seeing a bunch of errors to acquire the
jobserver token which are caused by interrupts at bad times.

I think it was a kernel update what caused this, fwiw, but not 100% sure.

Both can happen according to the man pages, and we're not handling them
sensibly.

I think retrying the read / poll when the error is EINTR is a sensible thing to
do.
@emilio
Copy link
Contributor Author

emilio commented Jan 13, 2020

Yeah, retrying the poll instead of the read would be equivalent if there's data to read I guess, so agreed, done :)

@alexcrichton alexcrichton merged commit 655977e into rust-lang:master Jan 13, 2020
@alexcrichton
Copy link
Member

Thanks!

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.

2 participants