Feature: urllib3 instead of curl

[spawn-guy]

for #737

i've migrated CurlClient to Urllib3Client and updated some unit tests.

i'd like to test this more thoroughly, though

needs a little bit more code cleanup as i am not even use this works (tests don't fail at least)

let me know what you think

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 16 lines in your changes missing coverage. Please review.

Project coverage is 81.47%. Comparing base (3f5d199) to head (e5ae986).
Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
kombu/asynchronous/http/urllib3_client.py	86.36%	10 Missing and 5 partials ⚠️
kombu/asynchronous/aws/ext.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
+ Coverage   80.99%   81.47%   +0.48%     
==========================================
  Files          77       77              
  Lines        9586     9500      -86     
  Branches     1168     1148      -20     
==========================================
- Hits         7764     7740      -24     
+ Misses       1620     1569      -51     
+ Partials      202      191      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Nusnus]

Hey there 👋
Can you please the CI failures?

Thanks 🙏

[auvipy]

this would be a good move. please also check the failing tests

[spawn-guy]

a problem i see is that urllib3.PoolManager doesn't support proxy per request. to support proxies i should instantiate urllib3.ProxyManager.
an idea popped-up - i can create two pools.. one with a proxy, the other without. but still: one pool == one proxy

i think i need some guidance on how to properly test this migration: Who uses this client Outside of "kombu in vacuum"?

also the curl files are not removed yet

[Nusnus]

I gave it a quick review, nothing special popped into my eyes so far.
I'll review again if the CI fully passes.

[Nusnus]

a problem i see is that urllib3.PoolManager doesn't support proxy per request. to support proxies i should instantiate urllib3.ProxyManager.

i think i need some guidance on how to properly test this migration: Who uses this client Outside of "kombu in vacuum"?

Good question. Maybe check Celery code?

also the curl files are not removed yet

Can you change the PR to draft meanwhile?
Let us know it's done when you set it back to "Ready for Review" 🙏

[spawn-guy]

i've run my code inside my beta environment (or at least i hope i did.. pipenv seems to checkout the correct commit id from my repo) (i even verified that the urllib3_client.py is, indeed, in the site-packages folder)

i've also tried pytest-celery and tests in celery (where there is a weird bug on Windows with some {docstring} and unicode characters - so i removed the {docstring}) and tests succeeded with Tests passed: 32,167, ignored: 160 of 32,331 tests

and all seems to behave as usual. but i don't use proxies

@auvipy @Nusnus the last questions are:

• what do we do with the proxy-per-request? a 1 urllib3.ProxyManager with 1 proxy per Pool ... or should i create some more pools per each proxy setting in requests (some lru cache, maybe)?
• should i continue to clean up the rest of the leftover curl code?

PS: the failing tests, basically, timeout during execution ;) = no more bad py3.8 code. and some code-coverage issue to fix

[auvipy]

• what do we do with the proxy-per-request? a 1 urllib3.ProxyManager with 1 proxy per Pool ... or should i create some more pools per each proxy setting in requests (some lru cache, maybe)?

may be the first approach would be fine to start. but if you can also go extra mile, it would be great as well

[spawn-guy]

Speaking about refactoring : if I remove the curl=None from request call - we can have a proper proxy pool. I just need to figure out which configure settings are used. But this is a breaking change 😬

[spawn-guy]

so.. apparently, for aws.getrequest - someone disabled certificate verification.

and now (with all my fancy code that enabled missing request.parameters) urllib3 complains that ssl verification is disabled :)
https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings

but then it complains that ca_cert is missing...

and all that worked with defaults :)

fixing that based on boto code + https://urllib3.readthedocs.io/en/latest/user-guide.html#ssl

[Nusnus]

Ping me when the CI is green.
I’ll give it at least a quick review to see nothing pops into my eyes.

Thanks!

[Nusnus]

Ready for review? @spawn-guy

[spawn-guy]

@Nusnus Ready 🤓

i have this running on my aws environment. and i was debugging the errors on it (and i was, indeed, getting urllib3 errors) - so i know this thing is Working 😝

but, i don't use any proxy (duh) - this is the part that can have errors in the field applications. and i'm ready to fix them

[Nusnus]

Sorry for the delay @spawn-guy
Left some comments.
But >90% looks fine.

Also, please check the code coverage warnings, some paths aren’t tested.
Let me know if that’s an issue to handle as it’s not 100% a blocker.

[Nusnus]

Amazing work!
Thank you very much!

I left a comment or two, check it out before we merge the PR.

[Nusnus]

@auvipy I plan to merge it tomorrow (per the comments I left in the PR) and then release a new pre-release for Kombu.
You have requested the CI will pass, it now passes.

Please let us know if there’s anything else required for your approval.
Thank you.