RUBY-1222 Use connect_timeout when determining address family #876
Conversation
estolfo
changed the title
estolfo
force-pushed
the
RUBY-1222-conn-timeout-2
branch
from
4665c0a to
6d9c94c
Compare
| # @since 2.0.0 | ||
| def timeout | ||
| @timeout ||= options[:connect_timeout] || CONNECT_TIMEOUT | ||
| @timeout ||= options[:connect_timeout] |
There was a problem hiding this comment.
If I understand well this timeout is still used but not for connexion, does it make sense to keep it connect_timeout then? Especially if there's no default any more, it'll be infinite for most people, and even if they think about setting the socket_timeout to avoid any trouble, there'll still be an infinite timeout in the code, because the default specified in the doc is not applied.
I think this should either be socket_timeout or keep the default, WDYT @estolfo ?
There was a problem hiding this comment.
Hi @jarthod
See the description here in our Server Discovery and Monitoring specification.
This timeout is applied only to the monitor thread's connection. It will be used both when connecting and for when regularly executing the ismaster command. I've put the default value of CONNECT_TIMEOUT back in, as you're right that if there is no connect_timeout option set, it would be infinite when sending the command on the socket.
The server connection will use the connect_timeout from the options (or the default) when connecting but then the socket_timeout when sending the operation, if one is specified. If one is not specified, it will be infinite.
Does that make sense?
Emily
There was a problem hiding this comment.
Totally makes sense, then it should have the default value as you did.
Maybe there should be just one constant though? to make sure nobody changes one and not the other?
There was a problem hiding this comment.
Yes, I agree we should have one constant but I thought it was awkward/bad design for the address to reference the Server::Monitor::Connection's constants or vice-versa. I'll do some more thinking and see if I can do use a connect timeout default constant better.
There was a problem hiding this comment.
Yes indeed, If it's used in multiple places I would probably move it up to the Mongo module or Configuration one, but that's a matter of taste and conventions you can do what you want ;)
There was a problem hiding this comment.
I put the constant in Server so that Address and Server::Monitor::Connection could reference it.
lib/mongo/server/connection.rb
Outdated
| def timeout | ||
| @timeout ||= options[:socket_timeout] | ||
| end | ||
| alias :socket_timeout :timeout |
There was a problem hiding this comment.
why alias instead of rename the method? is it used somewhere else? or just safety in case someone calls it from outside?
There was a problem hiding this comment.
Even though this is all private api, in general, I'd rather not remove or rename methods unless I'm planning to do a major release. And in that case, I'd mark it as deprecated leading up to the release.
Ideally, I would just rename it but I need to stick with semantic versioning rules.
There was a problem hiding this comment.
Yeah ok, so you'll mark it as deprecated then?
There was a problem hiding this comment.
I've marked timeout as deprecated in Server::Monitor::Connection and Server::Connection to favor the more explicit socket_timeout method name.
estolfo
changed the title
…etermining resolver
estolfo
force-pushed
the
RUBY-1222-conn-timeout-2
branch
from
dcc4870 to
e0d4e78
Compare
2 participants
No description provided.