- Notifications
You must be signed in to change notification settings - Fork923
fix: use raw syscalls to write binary we execute#11684
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
| // golang's standard OS library can sometimes leave the file descriptor open even after | ||
| // "Closing" the file (which can then lead to a "text file busy" error, so we bypass this | ||
| // and use syscall directly). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this a known / documented issue in the stdlib?
Would be nice to remove this workaround if it gets fixed in a later version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TBH, I think what's supposed to happen is that it only leaves the fd open if there is some pending I/O, like on another goroutine, which wouldn't apply here. But, behavior is different on different OSes, so it's hard to be sure, and I'm at a loss for another explanation of "text file busy".
I figured since we don't need any non-blocking IO or polling, it would be better to just simplify the syscalls we make, and see if we still see the flake.
Merge activity
|
Fixes flake seen here, I think
https://github.com/coder/coder/actions/runs/7565915337/job/20602500818
golang's file processing is complex, and in at least some cases it can return from a file.Close() call without having actually closed the file descriptor.
If we're holding open the file descriptor of an executable we just wrote, and try to execute it, it will fail with "text file busy" which is what we have seen.
So, to be extra sure, I've avoided the standard library and directly called the syscalls to open, write, and close the file we intend to use in the test.
I've also added some more logging so if it's some issue of multiple tests writing to the same location, the we might have a chance to see it.