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

Net::OpenTimeout is a Faraday::ConnectionFailed and not a Faraday::Ti… #980

Closed
wants to merge 1 commit into from

Conversation

grosser
Copy link
Contributor

@grosser grosser commented May 16, 2019

…meoutError

according to

exceptions << Net::OpenTimeout if defined?(Net::OpenTimeout)
open timeout is considered a failed connection, so let's make net-http-persistent behave like net-http ... or I can open the opposite PR to make both raise timeout 🤷‍♂

In general I'd think it would be nice to consider Net::OpenTimeout a timeout so that the retry middleware handles it by default, it was added in #438

@mislav @iMacTia @mike-bourgeous @rusikf

@grosser
Copy link
Contributor Author

grosser commented May 16, 2019

wtf ... Method perform_request has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring. ... want a PR to kick this out and use rubocop instead ?

@grosser
Copy link
Contributor Author

grosser commented May 16, 2019

also CI setup seems to be broken ? https://circleci.com/gh/lostisland/faraday/tree/master
... can anyone fix that ?
I had 1 error on master and this does not introduce any new errors 😓

@iMacTia
Copy link
Member

iMacTia commented May 16, 2019

Hi @grosser, thanks for raising this.
If I understand your point (and it is a very good point), I think it was extensively covered already here: #718

We also came up with a proposed solution, however there's a big issue here: your solution and the one proposed in that discussion are both breaking backwards compatibility.

For this reason, I'm afraid I'm unable to accept this PR at the moment as we decided that v1.0 will be fully backwards compatible and can only provide new features (like the streaming requests that you contributed to ❤️).

We'll address the issue with errors and attempt to standardise them in v2.0, but that work won't be limited to net-http-persistent

@iMacTia iMacTia closed this May 16, 2019
@grosser
Copy link
Contributor Author

grosser commented May 19, 2019

is anyone working on v2 ?
... would be nice to also make connections stateful by default, so we don't really need net-http-persistent at all since it would keep the connection open as long as the user uses the same connection object ... or at least make it not use this awkward global cache that erm someone contributed to fix it ...

@iMacTia
Copy link
Member

iMacTia commented May 19, 2019

No one actively working on v2 as the core team is focusing on getting v1 over the line at the moment. Works on v2 will likely start shortly after the release of v1, however we are beginning to collect the feature requests that will go into v2, as you can see from the milestone for the issue I linked above.

If you think there's something that we should seriously address in v2, you're more than welcome to open an issue for it and we'll mark it the same way so we won't forget.

Lastly, we really appreciate your contributions, and I'm sure you built some knowledge on Faraday, so I hope you'll be willing to lend some help once v2 works will start 👍

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