Skip to content

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Mar 15, 2024

Both Socket and IO::FileDescriptor have public read_timeout and write_timeout properties. For some reasons these ended up being defined in IO::Evented (which is a shared module between the two, but only on Unix systems), and IO::Overlapped (same job, but on Windows). That puts a general public API in places that are implementation-specific. The definitions are exactly identical, but they should never be duplicated in platform specific modules. They need to be defined in the public API space.

This patch moves the definitions to Socket and IO::FileDescriptor directly. This ends up still duplicated, but that's okay. Both types don't have any common ancestors except generic IO. It's just coincidental that the internal implementation is shared.

The code can also be simplified a by defining getters and setters with the property macro. That reduces the code a lot and leaves only the odd Number overloads (which might be good for deprecation? cf. #14368) preventing it from becoming a two-liner.

@straight-shoota straight-shoota self-assigned this Mar 15, 2024
@straight-shoota straight-shoota changed the title Move timeout properties to Socket and IO::FileDescriptor Move timeout properties to Socket and IO::FileDescriptor Mar 15, 2024
@straight-shoota straight-shoota added this to the 1.12.0 milestone Mar 17, 2024
@straight-shoota straight-shoota merged commit c0cb225 into crystal-lang:master Mar 18, 2024
@straight-shoota straight-shoota deleted the refactor/read-write-timeout branch March 18, 2024 10:37
@beta-ziliani beta-ziliani added the breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. kind:refactor topic:stdlib:files topic:stdlib:networking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants