feat(http): switch httpx for niquests in order to stabilize network I/O #375
Conversation
| with builtins.open(result_path, "wb") as file: | ||
| last_progress = current_progress | ||
| for chunk in response.iter_bytes(5 * 1024 * 1024): | ||
| for chunk in response.iter_raw(-1): |
There was a problem hiding this comment.
Niquests support -1 as the chunk size. It's just plain better, it allows to take whatever arrives immediately. Skipping the whole buffer logic.
| ) | ||
| dest = self._session.cfg.dav_endpoint + quote(full_dest_path) | ||
| headers = Headers({"Destination": dest, "Overwrite": "T" if overwrite else "F"}, encoding="utf-8") | ||
| headers = CaseInsensitiveDict({"Destination": dest.encode("utf-8"), "Overwrite": "T" if overwrite else "F"}) |
There was a problem hiding this comment.
Niquests does not have such thing as "Headers" in the way httpx implemented it.
We can just pass along bytes as values.
Just a note for the future, some reverse load balancer may not like receiving Unicode as header values.
| else: | ||
| phrase = "Unknown error" | ||
| raise NextcloudException(status_code, reason=phrase, info=info) | ||
| if not codes.is_error(status_code): |
There was a problem hiding this comment.
there is no strict equivalent at niquests httpx.codes
but the patch will act exactly as before.
There was a problem hiding this comment.
a note to be considered is that http/2 and http/3 no longer accept to return "response phrase" or also known as "reason" attached to the status code.
but you could already be aware of that.
|
|
||
|
|
||
| @dataclass | ||
| class Limits: |
There was a problem hiding this comment.
I added that for pure backward compatibility. Niquests as no such thing as a "Limits" object.
kwargs directly injected in the Session object.
| str_url = str(request.url) | ||
| if re.search(self._ocs_regexp, str_url) is not None: # this is OCS call | ||
| request.url = request.url.copy_merge_params({"format": "json"}) | ||
| request.url = patch_param(request.url, "format", "json") |
There was a problem hiding this comment.
niquests does not have an "URL" object. we are entitled to strict backward compatibility with requests, and bringing such object would not be taken gently by our users.
I created the tiny function patch_param that does essentially the same as before.
| a path to an SSL certificate file, or **False** (which will disable verification).""" | ||
| str_val = environ.get("NPA_NC_CERT", "True") | ||
| # https://github.com/encode/httpx/issues/302 | ||
| # when "httpx" will switch to use "truststore" by default - uncomment next line |
There was a problem hiding this comment.
And there it is! Niquests have a built in way to access the OS trust store! Without injection/monkeypatching!
| "niquests>=3,<4", | ||
| "pydantic>=2.1.1", | ||
| "python-dotenv>=1", | ||
| "truststore==0.10", |
There was a problem hiding this comment.
as I said earlier, no more need for that.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 95.11% 95.09% -0.03%
==========================================
Files 45 45
Lines 5324 5360 +36
==========================================
+ Hits 5064 5097 +33
- Misses 260 263 +3
🚀 New features to boost your workflow:
|
|
nice, thank your for this PR, I will try find time to review it soon and hopefully to merge |
|
Great! Regards, |
Fixes #365
Changes proposed in this pull request:
Quick history around that, httpx is being a bit stale for a couple of years now and report of spurious errors like the one in #365 is killing the global UX of that library. Not only that but httpx is absolutely unusable in the freethreaded build for example, it crash instantly on any attempt. Moreover the performance are often pointed out, especially in async. While Niquests isn't faster than aiohttp right now, it's better than requests and httpx.
Finally, a touch of hope, we recently were selected into the GitHub Secure Open Source Program (Session 2)! We manage several packages and one very high profile (charset-normalizer). Our security standards are solid and we currently are in the way of making every (of our own) package CRA compliant (charset-normalizer already is!).
This PR is a firm start into migrating from httpx, I will take the time to add comments using GitHub UI so that you can quickly understand why a change occurred this and there. I tried my best to keep the change surface as minimal as I could.
Before submitting this, I ensured that the tests were all good.
Regards,