Skip to content

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Oct 7, 2022

No description provided.

@ioquatix ioquatix merged commit 844a9df into master Oct 8, 2022
@ioquatix ioquatix deleted the io-timeout-default-nil branch October 8, 2022 01:02
@konsolebox
Copy link
Contributor

What was the reason this change was made? This probably disabled no-timeout in UNIXSocket.

@ioquatix
Copy link
Member Author

ioquatix commented May 12, 2024

Qundef can't be specified by Ruby code.

Can you explain your use case?

I'm on the fence about allowing users to disable IO#timeout. It serves as a safety net, and being able to disable it bypasses the safety net. If you don't need a safety net, then don't use it at all.

@konsolebox
Copy link
Contributor

@ioquatix My use case is I'm using Docker API's /events endpoint. Because of this change I have to specify UNIXSocket#timeout as nil and not just read_timeout so read timeout can be disabled. It gets more complicated when wrapping the socket around Net::BufferedIO because Net::BufferedIO doesn't forward assignments of #timeout= to the internal socket.

nil is supposed to mean "no timeout" and this change should have considered referring to another object like a symbol or a singleton instance of a class like Net::DefaultClass where it gets assigned to Net::DEFAULT instead.

I now have my workaround but it's still cumbersome in general. HTTParty users for example will now have to specify both timeout and read_timeout when calling get just because of this.

Moving forward maybe Ruby can introduce an explicit "no timeout" value for this like Net::NO_TIMEOUT where its value is Net::NoTimeoutClass.instance.

@konsolebox
Copy link
Contributor

Also maybe consider having this in Net::BufferedIO as well:

def timeout
  @io.to_io.timeout
end

def timeout=(timeout)
  @io.to_io.timeout = timeout 
end

@konsolebox
Copy link
Contributor

konsolebox commented May 14, 2024

Correction: (HTTParty).get(uri, timeout: nil, read_timeout: nil) might actually still not work unless HTTParty::ConnectionAdapter#connection is modified to set http.timeout to options[:timeout] as well. HTTParty::ConnectionAdapter#add_timeout? also will have to be modified to accept timeout.nil? if options[:timeout] will still have to be validated using it. So it should be changed to timeout.nil? || timeout.is_a?(Integer) || timeout.is_a?(Float).

To be fair, "no timeout" doesn't work in HTTParty at all at the moment because of add_timeout? not accepting nil timeout and other *_timeout's don't get assigned with a nil value.

@ioquatix
Copy link
Member Author

Sorry, I don't fully understand your use case. Are you able to give me a smallish reproducible example?

@ioquatix
Copy link
Member Author

Because of this change I have to specify UNIXSocket#timeout as nil and not just read_timeout so read timeout can be disabled

Who is setting the initial timeout and why? Instead of disabling it after the fact, why not just write io.timeout = nil?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants