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

Fix IO::FileDescriptor.new for closed fd #14697

Conversation

straight-shoota
Copy link
Member

Skips any operations for detecting/setting blocking mode in IO::FileDescriptor.new when the file descriptor is closed. In this case it doesn't matter anyway.

Resolves #14569

src/io/file_descriptor.cr Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor

Oh, the spec fails on Windows:

Unable to get info (Crystal::System::FileDescriptor): The handle is invalid. (IO::Error)

@straight-shoota
Copy link
Member Author

straight-shoota commented Jun 16, 2024

The issue was that system_closed? was not implemented on Windows. I fixed that.
Now this is not ideal because with blocking.nil? and an open handle, GetFileType is called twice (once in system_closed? and once in system_info). Perhaps we could refactor FileDescriptor#initialize to avoid that (i.e. delegate a larger chunk to the system implementations).

@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 17, 2024
@ysbaddaden
Copy link
Contributor

AFAIK we don't call IO::FileDescriptor.new much with blocking being nil in practice. I think only .from_stdio does.

@straight-shoota
Copy link
Member Author

blocking = nil is the default value for IO::FileDescriptor.new. But of course, this constructor is not used much. It's also possible to pass blocking: nil to File.new from user code, though.
But it's probably not used much and the gain is minimal. So no rush to optimize this. But it could be an enhancement somtime in the future. system_closed? is only called from the constructor, so it could easily lend itself for a refactor.

@straight-shoota straight-shoota merged commit e5032d0 into crystal-lang:master Jun 18, 2024
61 checks passed
@straight-shoota straight-shoota deleted the fix/file_descriptor-new-closed branch June 18, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crystal runtime crashes on closed file descriptor for STDIN
2 participants