search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.21k stars 1.61k forks source link

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

Closed straight-shoota closed 2 weeks ago

straight-shoota commented 3 weeks ago

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

ysbaddaden commented 2 weeks ago

Oh, the spec fails on Windows:

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

straight-shoota commented 2 weeks ago

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).

ysbaddaden commented 2 weeks ago

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

straight-shoota commented 2 weeks ago

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.