Repository navigation
Conversation
…tching requests globally
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've been using PyPDNS for a while and kept working around a few bugs locally
instead of reporting them. Finaly fixed them properly. Four independent commits,
only
pypdns/api.pytouched, no new dependency. Tell me if you'd rather reviewthem as seperate PRs.
f5c0c5b: NameError on every query withoutrequests_cacheCachedResponseis imported in atry/except ImportError, but was then used ina runtime
isinstance()inside the parsing loop. Without requests-cache thatraises
NameError, which the loop'sexcept Exceptionswallows and re-raises asPDNSError: Unable to decode JSON objecton valid JSON. Took me a while tofigure out it wasnt a parsing problem.
Now uses
getattr(response, 'from_cache', False), hoisted out of the loop soit's evaluated once per response instead of once per record.
CachedResponseisleft only in annotations, which never get evaluated, so the NameError can't come
back.
b69e613: configurable page size initer_queryPage size was hardcoded to 50. The cursor for page N+1 only exists in the
response of page N, so the requests are strictly sequential, and a big result
spends most of its time waiting. I measured ~35ms per round trip to circl.lu on a
warm connection, so 10k records is ~200 requests and roughly 7s of doing nothing.
page_sizeis keyword-only, defaults to 50, nothing changes for existingcallers. Below 1 it raises, otherwise an empty page with a cursor spins the loop
forever.
I couldn't check what the backend caps
dribble-paginate-countat, you'd know.If it clamps low the parameter is harmless but the gain is smaller than that.
2862cb0:zone_time_lastnever setSo
zone_time_lastalways reads backNone, andzone_time_firstgetsoverwritten with the wrong value. It survived because the typed record was built
from 14 copy-pasted blocks; replaced with a
{field: type}table and one loop.Same accepted types, same error messages.
Not a speedup, in case that's the assumption: a record with no optional field is
~20% slower to build (0.74 → 0.90µs), a full one is a wash. That's 1.6ms per 10k
records, so it doesn't matter either way.
728aa1c: cache and timeout parameters silently ignoredinstall_cache()replacesrequests.Sessionprocess-wide, so every otherlibrary in the app starts caching to sqlite. And the
CachedSession()builtright after takes no arguments, so both cache parameters go nowhere. With
cache_file=<tmp>/pdns.cacheandcache_expire_after=42:Two cache files and the one you named isn't the one in use. Now a single
CachedSession(cache_name=..., expire_after=...).Same commit, three smaller ones:
query(timeout=...)has been dead since33b82a2refactored the request into_query, which hardcodes 15. Threaded back down. Also added it torfc_queryand
iter_query, sincequeryis deprecated and fixing only the method youtell people to stop using doesn't help.
timeout=Nonestill means 15s, notforever.
sort_by='count'raised a bareKeyErrorwhen a record lackedcount(whichis optional in COF), and
sort_by='rdata'raisedTypeErroron a mix ofstrings and lists. Both are
PDNSErrornow.KeyError. Switched toRdataType.make()/to_text(), which also accepts the RFC 3597TYPE65535spelling the stringpath used to reject. Checked the output against the old code on 14 inputs
first, identical everywhere the old one worked. The int path was already fine,
only strings were broken.
Testing
The existing tests need credentials and don't cover any of this, so I ran against
a local server speaking the dribble protocol (basic auth, NDJSON, cursor
pagination, keep-alive), real sockets and real
CachedSession, not mocks. 42assertions, in two virtualenvs with and without requests-cache, plus
mypy .strict.
50k generated records with mixed rrtypes,
rdataas string and list, unicode inmeta: every field round-trips identical, no duplicates or losses across pagesizes 50/500/5000,
zone_time_firstandzone_time_lastdistinct on 10k ofthem. Timeout checked on a real socket (2s server latency,
timeout=1cuts at1.0s). Cache checked for file location, expiry, and that
requests.Sessionisleft alone.
Glad to contribute these as an offline test suite in a follow-up if useful, they
run on forks without secrets, which the current ones can't.
Breaking-ish
./http_cache.sqlitetocache_file, so existing caches getorphaned, and entries actually expire now instead of never.
query(timeout=...)takes effect, so a very small value will now actually timeout.
zone_time_firstreturns the right value, so anyone reading it to getzone_time_lastneeds to stop.KeyError/TypeError/ValueErrorbecomePDNSError/PDNSRecordTypeErrorinthe cases above.
No version bump, left that to you.