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

NetHttpPersistent: ConnectionError on OpenTimeout #1154

Closed
wants to merge 1 commit into from

Conversation

olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented May 4, 2020

Description

This PR adds a special-case from the net-http adapter to the net-http-persistent adapter.

open timeout was treated as a failure to connect, but our tests required it to be a timeout. Turns out, that was a call which was necessary before 4.0.0 of net-http-persistent.

There was a 4.0 released of net-http-timeout.

This change fixes the build by reraising as ConnectionError, just like in Faraday's net-http adapter.

Fixes #1152 by making the build pass.

Questions for reviewer

  • The 4.0.0 release requires Ruby 2.5 (I think that it mentions the 2.4 going out of support as a reason to be able to let go of the exception handling it had in itself)
  • This is a breaking change for this adapter

@olleolleolle olleolleolle force-pushed the net-http-persistent-new-exception branch from 7891b32 to 13d9257 Compare May 4, 2020 07:38
@olleolleolle olleolleolle changed the title Raise a ConnectionError on Net::OpenTimeout in net-http-persistent Raise a timeout error on Net::OpenTimeout in net-http-persistent May 4, 2020
@olleolleolle olleolleolle marked this pull request as ready for review May 4, 2020 08:01
@olleolleolle olleolleolle requested a review from iMacTia May 4, 2020 08:01
@olleolleolle
Copy link
Member Author

@iMacTia Does this rhyme with how the other adapters handle open timeout?

@olleolleolle
Copy link
Member Author

I took a look at the history, and learned that the net-http adapter treats it as a ConnectionError on purpose: 9ad0fcd

@olleolleolle
Copy link
Member Author

Learning more: There was a recent release of net-http-persistent, which got in a new change. drbrain/net-http-persistent@2d23622

@olleolleolle olleolleolle force-pushed the net-http-persistent-new-exception branch from 13d9257 to 6a90ae5 Compare May 5, 2020 20:21
@olleolleolle olleolleolle changed the title Raise a timeout error on Net::OpenTimeout in net-http-persistent NetHttpPersistent: ConnectionError on OpenTimeout May 5, 2020
  - open timeout is treated as a failure to connect
  - 4.0.0 and up supports this
@olleolleolle olleolleolle force-pushed the net-http-persistent-new-exception branch from 6a90ae5 to 1d1ff2e Compare May 5, 2020 20:25
Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Thanks @olleolleolle for tackling this.
I had a closer look at the whole change and I suspect this change is being backwards-incompatible.
Tests will be passing, however updating Faraday while using net-http-persistent < 4.0 will probably break, as it will raise a different exception from before in case of timeout?
With this change, we basically stop testing for v3.0 and change the code to fit only v4.0

Feel free to point out if I'm misunderstanding.

@olleolleolle
Copy link
Member Author

Yes, to make this work for < 3.x + 3.x + 4.x, we need a new conditional in the "rescue and re-raise" section which this PR changed (and its tests).

@iMacTia iMacTia added the parked Parked for future label Oct 14, 2020
@iMacTia
Copy link
Member

iMacTia commented Dec 31, 2020

I THINK this is now fixed in #1226, although to keep backwards-compatibility I've kept Faraday::Timeout as the exception instead of Faraday::ConnectionFailed

@iMacTia iMacTia closed this Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parked Parked for future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faraday::Adapter::NetHttpPersistent timeout tests failing with net-http-persistent 4.0.0
2 participants