Docs: Socket and IO::FileDescriptor fd ownership & soft deprecate their blocking args #15924
Conversation
ysbaddaden
changed the title
ysbaddaden
changed the title
src/io/file_descriptor.cr
Outdated
| # WARNING: changing the blocking mode can break the IO system requirements and | ||
| # cause the event loop to misbehave, for example block the program when a | ||
| # fiber tries to read from this file descriptor. |
There was a problem hiding this comment.
thought: This sounds like it might be quite a foot gun. Perhaps we should not make it so easy to break the event loop...
So I'm wondering, what are legitimate use cases for setting blocking to something other than what the event loop chose?
There's reconfiguring the adopted file descriptors, which may not have been originally created by the event loop. But apart from that I figure it might be related to using raw file descriptors outside the stdlib API. And for that use case, we should not need such an easily accessible method in the top-level API.
There was a problem hiding this comment.
I think it's acceptable as long as we properly document the behavior.
We can for example create an IO in Crystal, make sure it's (non-)blocking then take the fd/handle and give it to an external library that expects the given blocking mode.
There was a problem hiding this comment.
More problematic is the documentation for IO::FileDescriptor#blocking that states:
Returns whether I/O operations on this file descriptor block the current thread. If false, operations might opt to suspend the current fiber instead.
Because on WINDOWS a STDIO might emulate non-blocking, something that might be dropped when we have #15871.
There was a problem hiding this comment.
We can for example create an IO in Crystal, make sure it's (non-)blocking then take the fd/handle and give it to an external library that expects the given blocking mode.
That use case goes beyond the scope of the stdlib API. And it's clear that passing the fd/handle around directly needs special attention.
But #blocking in the stdlib API seems inconspicuous It might be too much exposed for its limited (remaining) relevancy. It's an invitation for a footgun.
There was a problem hiding this comment.
Then we can consider to deprecate the #blocking and #blocking= methods.
It would still be nice to have a mean to have a target abstract method to change the blocking mode of a system socket, but it doesn't have to be an instance method 👍
There was a problem hiding this comment.
Exactly. It could be something like IO::FileDescriptor.set_blocking(fd : Handle, blocking : Bool).
src/io/file_descriptor.cr
Outdated
| # | ||
| # NOTE: On Windows the handle should have been created with `FILE_FLAG_OVERLAPPED`. | ||
| # NOTE: The *blocking* arg is deprecated since Crystal 1.17. Use `#blocking=` | ||
| # to change the blocking mode instead. | ||
| def self.new(fd : Handle, blocking = nil, *, close_on_finalize = true) |
There was a problem hiding this comment.
suggestion: I think we should be able to duplicate this def with a @[Deprecated] annotation. There are no parameters with default value before blocking. There are a couple other similar defs like that.
There was a problem hiding this comment.
Oh my, thanks to the deprecated annotation, I just noticed that the event loops call IO::FileDescriptor with a blocking arg 😅
There was a problem hiding this comment.
Same for ORIGINAL_STDIN/OUT/ERR.
- The `blocking` args of File constructors now defaults to `nil`. - The polling evloops now set the fd to non blocking by default. - The IOCP evloop uses OVERLAPPED IO by default.
Explicits that the fd/handle will be configured to match the event loop requirements when adopted in the Socket types (its configuration will be changed). Explicits that `#fd` returns the fd/handle configured for the current event loop, and no specific configuration must be expected. Explicits that changing a socket's blocking mode can break the event loop, and as such must be treated carefully.
Explicits that the fd/handle will be configured to match the event loop requirements when adopted in the Socket types (its configuration will be changed). Explicits that `#fd` returns the fd/handle configured for the current event loop, and no specific configuration must be expected. Explicits that changing a socket's blocking mode can break the event loop, and as such must be treated carefully.
|
Back to draft. Waiting for #15930. |
ysbaddaden
force-pushed
the
fix/docs-for-socket-and-file-fd-ownership+soft-deprecate-blocking-args
branch
from
3c44c1d to
cc31269
Compare
Improves the documentation around the system file descriptors and handles in
IO::FileDescriptorandSocketin relation to "adopting" the fd/handle into the IO system, or accessing it. I hope it starts to clarify the intent.The documentation also soft deprecates the
blockingargument of their constructors.Sadly, I can't use the
@[Deprecated]annotation and the usual two methods trick (one without the arg, and a deprecated one with the arg) because the blocking arg is positional and comes after default arguments.My doc-fu ain't the best. If you're more capable and find better phrasing, feel free to edit the PR 🙇
Follow up to #15804, #15754, #15753 and #15750.