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

avoid bad file descriptor shutdown #895

Merged

Conversation

altendky
Copy link
Contributor

@altendky
Copy link
Contributor Author

To be blunt, I'm not the biggest fan of this reactive solution. I could also use a bit better understanding of the mechanics around this, but maybe those with authority to review here can bring that. For example, I think that this os.read() is blocking and waits so what happens if the file descriptor is closed while waiting instead of before initiating the read? Also, the os.read() may return fewer bytes than expected so there should probably be a loop until we either get as many as we wanted or otherwise give up. In digging further into related details a better solution may surface.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 8, 2022

Thanks @altendky !

The solution works for me. We could move forward with this, and see later if a deeper chante is required.

Would you mind rebasing from master, and add a line + your GitHub nickname in the changelog 🙏 ?

1 similar comment
@BoboTiG
Copy link
Collaborator

BoboTiG commented Jun 8, 2022

Thanks @altendky !

The solution works for me. We could move forward with this, and see later if a deeper chante is required.

Would you mind rebasing from master, and add a line + your GitHub nickname in the changelog 🙏 ?

@altendky altendky force-pushed the avoid_bad_file_descriptor_shutdown branch from 1c448c1 to bc5a910 Compare June 9, 2022 02:04
@BoboTiG BoboTiG merged commit 1dd7fa8 into gorakhargosh:master Jun 9, 2022
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.

None yet

2 participants