Refactor Socket blocking mode#15804
Merged
straight-shoota merged 7 commits intocrystal-lang:masterfrom May 26, 2025
Merged
Conversation
ysbaddaden
commented
May 20, 2025
| sock = UNIXSocket.new(handle: fd, type: type, blocking: blocking) | ||
| sock.sync = true | ||
| sock | ||
| end |
Collaborator
Author
There was a problem hiding this comment.
NOTE: we don't currently need the Crystal::EventLoop#socketpair to return the blocking arg, as it's only supported on UNIX where we can query the blocking mode, and only win32 needs to save the info as an ivar.
ysbaddaden
commented
May 20, 2025
ysbaddaden
commented
May 20, 2025
Member
|
I think it would be helpful to rebase the commit history and squash some commits (only this once 😅). Overall it looks fine, but the history is hard to follow with all the back-and-forth and fixups. We'll have it easier now for review and whenever we want to make sense of what happened here later on, if the commits are more comprehensible. |
The libevent, epoll and kqueue event loops don't carry over the blocking mode from the server socket to the client socket. They always set the client socket into non-blocking mode. The `TCPServer#accept?` and `UNIXServer#accept?` already behaved that way, but `Socket#accept?` used to carry the blocking mode.
88da2f3 to
7b86f75
Compare
Collaborator
Author
|
Here comes the rebase, with focused commits 😂 |
straight-shoota
approved these changes
May 22, 2025
ysbaddaden
added a commit
to ysbaddaden/crystal
that referenced
this pull request
May 24, 2025
9 tasks
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors the handling of the Socket blocking mode.
Overall the stdlib now asks the event loop to create the sockets, and the event loop (opinionated) might use the general helpers from
Crystal::System::Socketto create the sockets, then further refine it.The event loop also reports whether the
blockingmode was set, or not, because win32 doesn't have a mean to ask whether the blocking mode has been set and we must record it (WSAIsBlockingis deprecated and only available in older Winsock 1.1 DLL).Changes the
blockingarg fromfalsetonilby default, which means that each event loop now decides to set the mode to non-blocking or not (breaking change of undocumented, but expected, behavior).Moves the fd/handle creation to the event loops, so they can decide to set the overlapped flag (win32) or blocking mode (unix) as they need.
Improves the win32 IOCP event loop to not set the blocking mode because it only needs the overlapped flag; it also moves the creation of the port completion to the event loop.
Reworks the
*Socketinitializers to not set theblockingarg for internal constructors (already set).The public
*Socketinitializers from a raw fd/handle still set the blocking mode, whennilit asks the event loop.FIXME: We probably want to keep theirblockingarg tofalseby default? We should ask the event loop when it'snil.NOTES:
The
blockingarg defaulting tonilisn't an immediate breaking change on UNIX: the libevent, epoll and kqueue event loops will keep setting the non-blocking mode. However, on win32, we won't set the socket to non-blocking anymore (no need, we use overlapped), and theio_uringevent loop will keep the socket in blocking mode; these are behavior changes.We should deprecate/remove support for the
blockingarg. OnlySocketandTCPSocketactually have it, neither ofTCPServer,UNIXSocketorUNIXServerdo, for unknown reasons.If we need to set the mode, for example to interact with an external C library, we can manually call
#blocking=.I tried to use the deprecated annotation, but we can't because the blocking arg comes after args with default values, and we can't use the usual distinct methods trick (one with/out the arg) 😭
Extracted from #15685.