Replace http gem with faraday

[andrzej-stencel]

This pull request replaces the http gem with Faraday, in line with a previous PR that replaced rest-client with Faraday. After this, the only HTTP library used by Kubeclient will be Faraday. This should satisfy #237.

Please let me know what you think, I'm happy to take any advice that will help me push this to completion.

[cben]

Code looks excellent 👏 👍 Haven't tested this yet.

Now that it uses same library as base Client, I have a feeling we could share more code,
but not a blocker.

[cben]

The main purpose of finish method was to allow interrupting a watch from another thread.

The previous implementation by @http_client&.close was kinda violent but worked — closing the client would cause the code in each to crash in various ways, depending on where exactly it was when it gets closed, and rescue StandardError would swallow the crash (when caused by finish).

A synchronous next if @finished here is much simpler & safer, but I think it'll only stop the loop after it had something to read, which can take unbounded time (until server sends next update)?

[andrzej-stencel]

Hm yes that's a very good observation. I'll try to confirm this with a test and then fix it, hopefully having come up with a solution. ;)

[andrzej-stencel]

@cben I have added the test that tries to close the watch connection from another thread when there are no updates from the API server and, just as you described, the WatchStream#finish method does not do its job in this scenario.

I then added a call to Faraday::Connection#close in the body of the WatchStream#finish method, but this didn't help. I'm afraid this is due to the #close method not being implemented in the Faraday adapter for net/http.

I'm not sure what we can do with this to be honest. Is there a different way to forcibly close the connection? I couldn't find one. Can we get rid of the finish functionality? Or should we rather change the HTTP libarary to be able to support the #finish use case? We could try some other Faraday backend or switch from Faraday to something else...

[cben]

Thanks for looking into this. Neat test!

Hmm, I'd like to play with this too.
We don't really need finish to close it immediately, just "soon" (this was already the semantics but should document it).
Perhaps a non-blocking read with a timeout might help?

I think this is not an immediate deal-breaker. We can in principle release a breaking 5.0 without finish and hopefully restore it in 5.1...

Can you mark the test pending/skipped and document that finish no longer works across threads, and I'll merge this.
Or maybe remove it completely — not really useful from inside the watching thread (can just return / break from the block), and it's better if someone upgrading to 5.0 but still calling finish to get an error rather than getting stuck? WDYT?

[cben]

Faraday documents close as:

# Close any persistent connections. The adapter should still be usable
# after calling close.

-- https://github.com/lostisland/faraday/blob/e41668ee591735a8be65fd739cddb7a27518eabd/lib/faraday/adapter.rb#L63-L68

And as far as I can tell, neither Faraday::Adapter::NetHttpPersistent nor Faraday::Adapter::NetHttp implement close, they just inherit the empty base implementation ☹️

Is there some way to reach in and actually close the connection? Or close underlying OS socket? ("if violence doesn't help, you're not using enough of it")

• With persistent connections, does that risk breaking some unrelated request that was going to reuse same connection?

BTW, is the test failure with persistent adapter or NetHttp?

[andrzej-stencel]

And as far as I can tell, neither Faraday::Adapter::NetHttpPersistent nor Faraday::Adapter::NetHttp implement close, they just inherit the empty base implementation ☹️

Yes, exactly!

Is there some way to reach in and actually close the connection? Or close underlying OS socket? ("if violence doesn't help, you're not using enough of it")

Couldn't find a way, but not an expert in either Ruby or OS/network programming. I wonder if @grosser could help here?

BTW, is the test failure with persistent adapter or NetHttp?

I only tested this with net/http but as noted above, I strongly believe net/http/persistent will behave in the same way as it does not implement the close method either.

[andrzej-stencel]

We don't really need finish to close it immediately, just "soon" (this was already the semantics but should document it).
Perhaps a non-blocking read with a timeout might help?

That's definitely an interesting notion, I'm not sure how much this would involve but I'll try to investigate this.

I think this is not an immediate deal-breaker. We can in principle release a breaking 5.0 without finish and hopefully restore it in 5.1...

Yes, it is a possibility, but certainly not the most pretty...

[grosser]

Best open an issue in faraday for the close, might be an easy fix. I touched a bit of code in there, but these edge-cases are always tricky :(

…

On Tue, Jan 19, 2021 at 6:21 AM Andrzej Stencel ***@***.***> wrote: We don't really need finish to close it immediately, just "soon" (this was already the semantics but should document it). Perhaps a non-blocking read with a timeout might help? That's definitely an interesting notion, I'm not sure how much this would involve but I'll try to investigate this. I think this is not an immediate deal-breaker. We can in principle release a breaking 5.0 without finish and hopefully restore it in 5.1... Yes, it is a possibility, but certainly not the most pretty... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#488 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACYZ4OF5OGFAYSSX75EA3S2WINJANCNFSM4VTATX2A> .

[grosser]

this can be closed, master uses faraday already ?

[cben]

No, master uses faraday where we previously used rest-client, but watching still uses http.
It'd great to converge them, although not strictly a release blocker.

[cben]

Turns out on_data response streaming is presently supported by only 2 faraday adapters — the basic Net::HTTP and httpx.
But it would be sad to generally support only these 2.

UPDATE: 3/10 as of Jan 2022, Typhoeus too.

• Could we allow watches to use to use a different adapter from other request types [maybe hardcoded Net::HTTP]?
• Should we look for non-faraday solution to watches?

[DocX]

Turns out on_data response streaming is presently supported by only 2 faraday adapters — the basic Net::HTTP and httpx.
But it would be sad to generally support only these 2.

What prevents kubeclient to use the specific adapter for its own use? Even if the user's app uses faraday with different adapter, it does not have to interfere with what kubeclient uses. The adapter can be configured everytime we create the faraday instance with Faraday.new { |f| f.adapter = ... }.

We already use different client if we don't use faraday in the watch code anyway, so the argument that it would not be aligned with the user's code is IMHO gone.

Whatever kubeclient uses is just internal to kubeclient.

I guess the only slight disadvantage is that if user uses adapter A, adding kubeclient would add adapter B to their gems? Which already happens with http now.

[cben]

Nothing prevents, your analysis is right.