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

Allow passing in timeout/retry settings to Net::HTTP #677

Conversation

tjschuck
Copy link

When fetching remote XML files from arbitrary URLs, you might want to configure different values for timeouts/retries to avoid allowing users to DoS you via intentionally slow endpoints. The Net::HTTP defaults are 60 seconds plus 1 retry, which could easily deplete resources if intentionally exploited.

When fetching remote XML files from arbitrary URLs, you might want to configure different values for timeouts/retries to avoid allowing users to DoS you via intentionally slow endpoints.  The Net::HTTP defaults are 60 seconds plus 1 retry, which could easily deplete resources if intentionally exploited.
@tjschuck
Copy link
Author

Just bumping this — @pitbulk anything I can do to help get this merged and shipped?

@johnnyshields
Copy link
Collaborator

@pitbulk this looks reasonable to merge.

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 9, 2024

@johnnyshields I think its ok, but I gonna prefer request_options to be the last paramter of parse_remote and parse_remote_to_array methods, rather than include them before the options parameter.

@tjschuck can you do the change?

@tjschuck
Copy link
Author

tjschuck commented Jul 9, 2024

@pitbulk Pushed!

Thanks @pitbulk and @johnnyshields

@johnnyshields
Copy link
Collaborator

Sorry to bikeshed this, I misread @pitbulk's earlier comment. I was thinking we would make these just part of options. But I guess it's fine this way too.

@tjschuck
Copy link
Author

@johnnyshields That's fine with me too. I honestly can't remember why I did it this way...

If you want me to push it into options, just let me know and I can do that shortly.

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 10, 2024

@johnnyshields I think it's ok to have both options differentiated. As we are introducing now all related to the request, I'm not against to leave its own place. Wonder if there is a way to inject them in the request rather than initialize each parameter one by one.

 http.open_timeout = request_options[:open_timeout] if request_options[:open_timeout]
 http.read_timeout = request_options[:read_timeout] if request_options[:read_timeout]
 http.max_retries = request_options[:max_retries] if request_options[:max_retries]

def parse_remote_to_hash(url, validate_cert = true, options = {})
parse_remote_to_array(url, validate_cert, options)[0]
def parse_remote_to_hash(url, validate_cert = true, options = {}, request_options = {})
parse_remote_to_array(url, validate_cert, request_options, options)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops this is wrong position

@johnnyshields
Copy link
Collaborator

johnnyshields commented Jul 10, 2024

Using one options arg avoids human errors like the one I just caught. I strongly prefer it. @pitbulk may we do it?

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 10, 2024

Also this should go on branch V2.0.0 where we force new ruby version

@johnnyshields
Copy link
Collaborator

@tjschuck I'll take this one over.

@tjschuck
Copy link
Author

@johnnyshields Go nuts! Thanks.

@johnnyshields
Copy link
Collaborator

Replaced by #709

@tjschuck tjschuck deleted the allow-timeout-config-for-remote-xml-fetching branch October 21, 2024 21:39
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.

3 participants