Adding Ability to Set Connection Timeout Values #289
Conversation
| return nil if url.nil? or url.empty? | ||
| uri = URI.parse(url) | ||
| http = Net::HTTP.new(@host, @port) | ||
| return nil if (url.nil? || url.empty?) |
There was a problem hiding this comment.
Don't use parentheses around the condition of an if.
There was a problem hiding this comment.
usually right, but currently wrong houndci-bot ... this can cause issues with compound if's . i would leave this
| @@ -129,15 +128,15 @@ def execute(options = {}) | |||
| @error = "Error parsing response: #{exc.message}" | |||
| end | |||
|
|
|||
| if !(@success or @error) | |||
| if !(@success || @error) | |||
There was a problem hiding this comment.
Favor unless over if for negative conditions.
lib/nexpose/api_request.rb
Outdated
| # If an explicit timeout is set, don't retry. | ||
| retry unless options.key? :timeout | ||
| if options.key?(:timeout) | ||
| retry if (time_tracker + options[:timeout].to_i) > Time.now |
There was a problem hiding this comment.
Avoid more than 3 levels of block nesting.
| # drops our HTTP connection before processing. We try 5 times to establish a | ||
| # connection in these situations. The actual exception occurs in the Ruby | ||
| # http library, which is why we use such generic error classes. | ||
| rescue OpenSSL::SSL::SSLError => e |
There was a problem hiding this comment.
I know code inspection complains about these but capturing the object makes runtime debugging easier, I would prefer they be kept.
| @password = pass | ||
| @token = token | ||
| @silo_id = silo_id | ||
| unless trust_cert.nil? |
There was a problem hiding this comment.
Sorry to nit pick this is another case where code inspection is not friendly to debugging, by making this a single line call you can lose break point granularity.
There was a problem hiding this comment.
can you give an example of what you mean? are you saying you'd want to put a breakdown on the inside the unless statement? if that's the case, you'd be able to just put the breakpoint inside the create_trust_store method.
There was a problem hiding this comment.
Again is just a convenience thing to me being able to break when trust_cert.nil? without setting a conditional on the break point that is already in the code and see @trust_store before and after the call to create_trust_store(trust_cert)
In this case it may not be all that useful since create_trust_store is local code, I am just adding context about some things that rubocop thinks are bad that do little to impact performance but can help debugging and readability. Especially if the method called is from some utility library that is used heavily.
There was a problem hiding this comment.
Fair enough, I see what you're saying. Personally, I am using breakpoints with binding.pry so for your example I would probably do binding.pry if trust_cert.nil? on a new line. Where I am assuming if we were to talk about "clicking an IDE" to create a breakpoint, that's what you're referring to.
That's a fair enough concern, and I'll keep it in mind going forward. On this particular line, do you have a preference if we leave it as is, or should I change it back?
There was a problem hiding this comment.
This is fine to leave as is.
lib/nexpose/api_request.rb
Outdated
| if @conn_tries < 5 | ||
| @conn_tries += 1 | ||
| retry | ||
| end | ||
| rescue ::Timeout::Error | ||
| $stdout.puts @http.read_timeout |
There was a problem hiding this comment.
I think this is a debugging statement that needs to be removed.
There was a problem hiding this comment.
yeah it will be, i'm still testing this branch
lib/nexpose/api_request.rb
Outdated
| # If an explicit timeout is set, don't retry. | ||
| retry unless options.key? :timeout | ||
| if options.key?(:timeout) | ||
| retry if (time_tracker + options[:timeout].to_i) > Time.now |
There was a problem hiding this comment.
This may not really be about this PR but more a general thought, should the retry here even occur now? Originally this was retried up to 5 times if no :timeout value was ever set, since there is now a default :timeout and the request surpassed it, would it make sense to simplify this now to just end at a single timeout either the default or user specified one?
There was a problem hiding this comment.
Scott and I were talking about not having retries at all anymore, maybe. At least as far as time out retries go, they are often more of a problem than just failing as many Nexpose API endpoints will do a lot of work on a heavy-load console before returning a response. Retrying in that case just repeats the work being done which we don't want to do IMO.
lib/nexpose/api_request.rb
Outdated
| if @conn_tries < 5 | ||
| @conn_tries += 1 | ||
| retry | ||
| end | ||
| rescue ::Timeout::Error | ||
| rescue ::Timeout::Error => error |
There was a problem hiding this comment.
Useless assignment to variable - error.
| end | ||
| @error = "Nexpose did not respond within #{@http.read_timeout} seconds." | ||
| rescue ::Timeout::Error => error | ||
| @error = "Nexpose did not respond within #{@http.open_timeout} seconds. See <README> for information on setting the different Timeout values." |
There was a problem hiding this comment.
will link to the wiki documentation once it's ready
lib/nexpose/connection.rb
Outdated
| attr_accessor :timeout | ||
| # The optional HTTP open_timeout value | ||
| # For more information visit the link below: | ||
| # https://ruby-doc.org/stdlib-<YOUR_RUBY_VERSION>/libdoc/net/http/rdoc/Net/HTTP.html#open_timeout-attribute-method |
There was a problem hiding this comment.
Can we make this link version-agnostic (i.e. latest) or just pick a version? might be confusing to newbies since it's not a valid link
:timeoutand:open_timeoutto Nexpose::Connection.:timeoutand:open_timeoutare set to 120 seconds.$stderrwhen rescuing uncommon errors, asking users to open Github issues if these errors are seen in the wild.