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

Breaking change to Crystal::System::Socket#system_connect timeout type #14752

Closed
nobodywasishere opened this issue Jun 25, 2024 · 5 comments · Fixed by #14755
Closed

Breaking change to Crystal::System::Socket#system_connect timeout type #14752

nobodywasishere opened this issue Jun 25, 2024 · 5 comments · Fixed by #14755
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:stdlib:networking

Comments

@nobodywasishere
Copy link
Contributor

Bug Report

Related to #14368.

In #14643, Crystal::System::Socket#system_connect was changed to no longer automatically convert Number types (using .seconds) to Time::Spans, instead explicitly requiring them. This is a breaking change. To have this be deprecated instead, there should've been an additional method overload that did the automatic conversion with the @[Deprecated] annotation.

@Blacksmoke16
Copy link
Member

Crystal::System is an internal implementation detail. So while this may be a breaking change, unless it affected some public API, it is allowed. Was how you discovered this using system_connect directly or?

@nobodywasishere
Copy link
Contributor Author

system_connect is used by Socket#connect, so this is a breaking change for it and its subclasses (IPSocket, TCPSocket, UDPSocket, etc.)

@Blacksmoke16 Blacksmoke16 added topic:stdlib:networking kind:regression Something that used to correctly work but no longer works labels Jun 25, 2024
@ysbaddaden
Copy link
Contributor

We won't fix #system_connect since it's undocumented internal API, but there should indeed be a deprecated override method for Socket#connect(addr, timeout = nil, &) to not break the public API 👍

@straight-shoota
Copy link
Member

Thanks for reporting and fixing. Apparently this got lost in the heat of the moment.

@straight-shoota
Copy link
Member

Actually, I believe the correct and direct fix would be to move the original conversion from #system_connect to #connect:

--- i/src/socket.cr
+++ w/src/socket.cr
@@ -110,6 +110,7 @@ class Socket < IO
   # Tries to connect to a remote address. Yields an `IO::TimeoutError` or an
   # `Socket::ConnectError` error if the connection failed.
   def connect(addr, timeout = nil, &)
+    timeout = timeout.seconds unless timeout.is_a? ::Time::Span | Nil
     result = system_connect(addr, timeout)
     yield result if result.is_a?(Exception)
   end

I've had this in my development branch, but it must've gotten lost somewhere during refactoring and rebasing.

After this we can move forward with adding type restricitons to timeout and deprecating the overload which is not Time::Span?.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug kind:regression Something that used to correctly work but no longer works topic:stdlib:networking
Projects
None yet
4 participants