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

[rush](package-deps-hash) Avoid defect to be produced when spawned "git hash-object" process stdin fails while computing repo-state #4478

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

antoine-coulon
Copy link
Contributor

Summary

Fixes #4475

As explained in #3517, symlinked directories can't be hashed by git hash-object. Whatever the underlying reason is for that, as I already stated in the issue, executed commands should not crash due to this problem and just emit a warning.

Note that we might consider this to be a workaround while we fix the root cause which is discarding symlinked directories from the analysis. However I feel like having error handlers there will be worth it anyway as other errors could occur in the future and we definitely don't want to crash the process at this step which is not mandatory for Rush to properly work (file diffing).

Details

I solved the problem by simply attaching an "error" handler on the underlying child process' stdin stream to avoid EPIPE errors (fatal) to occur. This error happens because the git child process closes because of the mentioned errors, but the stdin Readable keeps pushing file paths while the Writable is closed.

How it was tested

Mainly through manual testing using https://github.com/antoine-coulon/rushelp and the Docker container created from it. The issue can easily be re-created, just run the container, move to /build and run rush my-bulk-command, EPIPE will occur. This is because git status includes /bin and /lib symlinked directories so as I said git child process will emit errors.

Impacted documentation

None

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

Updating the wording of the comments for clarity about the exact issue being fixed and how the change mitigates it.

@iclanton iclanton merged commit b445758 into microsoft:main Jan 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants