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

Extract system-specifics from Socket #10706

Merged

Conversation

straight-shoota
Copy link
Member

System-specific parts of Socket are extracted to Crystal::System::Socket.
This is just a refactor with almost no semantic changes.

The basic system API is already verified to be compatible with the upcoming win32 implementation.

There are still a couple of LibC's left, mostly for simple socket option getter and setters. I expect most should work with the winsock API as well, so it may not be necessary to abstract them. In any case, at this point the most suitable abstraction is not clear, and would need to be determined later in the porting process.

Comment on lines -556 to -558
# Clear the @volatile_fd before actually closing it in order to
# reduce the chance of reading an outdated fd value
_fd = @volatile_fd.swap(-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. After looking at evented_close, I'm wondering a bit if it would make sense to split that method in two, one part that removes all pending events that would happen before closing, and one that wake up the result, called after everything. As @closed isn't volatile it seems strange to wake up stuff (that can be run in other threads) before we actually is done. Setting @closed to false like that is definitely safe in the single threaded mode, but in multithreaded I wonder how far the assignment to @closed may be moved around by the compiler and CPU.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds legit, but I'd prefer to leave such restructuring for a follow-up, to keep the complexity of this simple yet extensive refactoring low.

@straight-shoota
Copy link
Member Author

I added two more commits extracting behaviour specific to IP and TCP sockets. There are still a couple of system-specifics left (in Socket and UDPSocket mostly), but they're more special methods and I'd leave them for later when they get ported. Maybe it would even be fine to leave them as is when POSIX and win32 use the same constants.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@straight-shoota straight-shoota added this to the 1.1.0 milestone May 19, 2021
@straight-shoota straight-shoota merged commit a2c632e into crystal-lang:master May 20, 2021
@straight-shoota straight-shoota deleted the refactor/socket-system branch May 20, 2021 12:58
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.

3 participants