Skip to content

Batch of bug fixes: cache, timeout, record parsing, pagination - #45

Open
Sn0wAlice wants to merge 4 commits into
CIRCL:mainfrom
Sn0wAlice:main
Open

Sn0wAlice wants to merge 4 commits into
CIRCL:mainfrom
Sn0wAlice:main

Conversation

@Sn0wAlice

Copy link
Copy Markdown

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.py touched, no new dependency. Tell me if you'd rather review
them as seperate PRs.

f5c0c5b: NameError on every query without requests_cache

CachedResponse is imported in a try/except ImportError, but was then used in
a runtime isinstance() inside the parsing loop. Without requests-cache that
raises NameError, which the loop's except Exception swallows and re-raises as
PDNSError: Unable to decode JSON object on valid JSON. Took me a while to
figure out it wasnt a parsing problem.

Now uses getattr(response, 'from_cache', False), hoisted out of the loop so
it's evaluated once per response instead of once per record. CachedResponse is
left only in annotations, which never get evaluated, so the NameError can't come
back.

b69e613: configurable page size in iter_query

Page 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_size is keyword-only, defaults to 50, nothing changes for existing
callers. 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-count at, you'd know.
If it clamps low the parameter is harmless but the gain is smaller than that.

2862cb0: zone_time_last never set

if 'zone_time_last' in self._raw_record:
    ...
    to_return['zone_time_first'] = self._raw_record["zone_time_last"]
    #          ^^^^^^^^^^^^^^^ wrong key

So zone_time_last always reads back None, and zone_time_first gets
overwritten 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 ignored

install_cache() replaces requests.Session process-wide, so every other
library in the app starts caching to sqlite. And the CachedSession() built
right after takes no arguments, so both cache parameters go nowhere. With
cache_file=<tmp>/pdns.cache and cache_expire_after=42:

http_cache.sqlite   <- what the session actually used, in CWD
pdns.cache          <- created by install_cache, then ignored
expire_after: asked 42, effective -1 (never expires)
requests.Session: Session -> _ConfiguredCachedSession

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 since 33b82a2 refactored the request into
    _query, which hardcodes 15. Threaded back down. Also added it to rfc_query
    and iter_query, since query is deprecated and fixing only the method you
    tell people to stop using doesn't help. timeout=None still means 15s, not
    forever.
  • sort_by='count' raised a bare KeyError when a record lacked count (which
    is optional in COF), and sort_by='rdata' raised TypeError on a mix of
    strings and lists. Both are PDNSError now.
  • An unknown rrtype escaped as KeyError. Switched to RdataType.make() /
    to_text(), which also accepts the RFC 3597 TYPE65535 spelling the string
    path 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. 42
assertions, in two virtualenvs with and without requests-cache, plus mypy .
strict.

50k generated records with mixed rrtypes, rdata as string and list, unicode in
meta: every field round-trips identical, no duplicates or losses across page
sizes 50/500/5000, zone_time_first and zone_time_last distinct on 10k of
them. Timeout checked on a real socket (2s server latency, timeout=1 cuts at
1.0s). Cache checked for file location, expiry, and that requests.Session is
left 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

  • Cache moves from ./http_cache.sqlite to cache_file, so existing caches get
    orphaned, and entries actually expire now instead of never.
  • query(timeout=...) takes effect, so a very small value will now actually time
    out.
  • zone_time_first returns the right value, so anyone reading it to get
    zone_time_last needs to stop.
  • KeyError/TypeError/ValueError become PDNSError/PDNSRecordTypeError in
    the cases above.

No version bump, left that to you.

@Rafiot Rafiot self-assigned this Sep 23, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants