Add Crystal::FdLock + integration
#16128
Conversation
ysbaddaden
force-pushed
the
feature/add-crystal-fd-lock
branch
from
f270b4b to
8dcd1da
Compare
|
The GC prints a bunch of I'm at a loss to guess where the cycle can come from. |
ysbaddaden
force-pushed
the
feature/add-crystal-fd-lock
branch
from
68e1929 to
47bc6ef
Compare
| end | ||
|
|
||
| def close(file_descriptor : Crystal::System::FileDescriptor) : Nil | ||
| def before_close(file_descriptor : Crystal::System::FileDescriptor) : Nil |
There was a problem hiding this comment.
thought: The names before_close and close are not very descriptive about their actual purpose. Maybe we can find more explicit names?
before_close closes the wrapper object, while close closes the system resource. Maybe close_wrapper and close_system_fd? Not entirely convinced these are the best choices, but they seem better than the current ones...
There was a problem hiding this comment.
I'm not sure what you mean with "wrapper object". Maybe #before_close could be #resume_all but it becomes a directive (do this) rather than a broader "we're about to close, do whatever you need to"
But #close becomes more to the point: close this fd/sock (and only useful for io_uring that can close async).
There was a problem hiding this comment.
Maybe "wrapper object" is not a good term. I was thinking about FileDescriptor as being a wrapper of the system API. And before_close takes care of closing the aspects specific to the runtime, while close closes the actual file descriptor resource for the OS.
There was a problem hiding this comment.
Note that the behavior of #before_close here is identical to the #reopened hook called after we reopen a file descriptor: resume any pending waiter for the given IO::FileDescriptor or Socket. We might as well have a single #resume_all, maybe?
src/crystal/fd_lock.cr
Outdated
|
|
||
| private def decrement_slow(m) | ||
| # last ref after close must resume the closing fiber | ||
| return unless (m & CLOSED) == CLOSED && (m & MASK) == REF |
There was a problem hiding this comment.
suggestion: Simplify to CLOSED | REF
| return unless (m & CLOSED) == CLOSED && (m & MASK) == REF | |
| return unless m == CLOSED | REF |
There was a problem hiding this comment.
It could, but I prefer a more explicit "this is closed" + "this is the last ref", and it won't break if we add more bits later.
There was a problem hiding this comment.
For me it was a bit surprising because the expression is more complicated than it needs to be. The simplified version is much easier to understand that this matches actually one single state.
|
@straight-shoota I'll see the suggestions after I figure out the issues (especially the GC finalization cycles). |
|
Now the interpreter is broken 😮💨 |
|
I made FdLock a NOOP until the |
On UNIX also delegates the implementation from `System::Socket` to `System::FileDescriptor` since it's identical.
Instead use class methods on System::Socket on Win32 for the one case where we need to configure the socket handle after accept.
|
There's an issue on Windows. I should rebase from #16137 an investigate. |
Implements a reference counted lock to eventually protect IO objects such as `IO::FileDescriptor`, `File` and `Socket` that take a reusable `fd` internally to protect close against thread safety race conditions since once closed, the fd will be reused by the OS.
ysbaddaden
force-pushed
the
feature/add-crystal-fd-lock
branch
from
555f008 to
9301e5b
Compare
First pass at fixing #16127 using a basic ref counter and delayed close to when there are no more references.
The integration is limited to UNIX, needs to refactor a few system methods, and requires a new
EventLoop#before_close, distinct fromEventLoop#close, so we can close the fd lock, resume pending waiters (references can be decremented) then we can finally close the fd for real.The refcount only happens for calls that may affect the
fd, mere queries for thefdconfiguration skip it: at worst they return whatever config or fail normally.closes #16127