Fix map mis-pairing and build RESP3 maps with one insertion per pair

[mmick78]

Summary

tryParentize builds RESP3 map replies by parking the key in the target dict under a Py_None placeholder and recovering it with popitem():

case REDIS_REPLY_MAP:
    if (task->idx % 2 == 0) {
        /* Set a temporary item to save the object as a key. */
        int res = PyDict_SetItem(parent, obj, Py_None);
        ...
    } else {
        /* Pop the temporary item and set proper key and value. */
        PyObject *last_item = PyObject_CallMethod(parent, "popitem", NULL);
        PyObject *last_key = PyTuple_GetItem(last_item, 0);
        PyDict_SetItem(parent, last_key, obj);
        ...
    }

This is incorrect whenever a key repeats and is not the most recently inserted entry. Re-setting an existing key updates it in place rather than moving it to the end, so popitem() removes somebody else's pair: the value lands on the wrong key, and the Py_None placeholder is left behind in the returned dict.

>>> r = hiredis.Reader()
>>> r.feed(b"%3\r\n+a\r\n:1\r\n+b\r\n:2\r\n+a\r\n:3\r\n")
>>> r.gets()
{b'a': None, b'b': 3}     # expected {b'a': 3, b'b': 2}

Keys that merely hash equal collide the same way, so a map containing both 0 and False, or 1 and True, is enough to trigger it.

It is also slow: every pair costs a hash and insert, a name-based method lookup on the dict, a tuple allocation, a delete, and a second hash and insert — where one insert would do.

This PR holds the pending key in the reader and inserts it once, when its value arrives. That is both correct and substantially faster.

Correctness

I validated both builds against an independent pure-Python RESP3 parser that builds maps with ordinary dict assignment, over a randomized corpus of 20,000 replies (every RESP3 type, nesting to depth 5, both decode_responses modes):

build	parses checked	mismatches vs. oracle
master	39,052	952
this PR	39,052	0

Every one of the 952 is the mis-pairing above.

I also diffed this build directly against master on 20,000 randomized replies fed at random chunk boundaries. There is no case where both builds parse successfully and return different data, other than the ones where master is wrong. The remaining differences are confined to input that fails to parse anyway — see the behavioural note below.

Performance

Reader.gets() on a map reply, best of 5 runs (script below). "RESP2 arr+dict" is the same data as a RESP2 flat array converted with dict(zip(it, it)) in Python, i.e. what a client gets by staying on RESP2.

fields	before	after	speedup	vs. RESP2 arr+dict, before → after
10	1.8 µs	0.7 µs	2.6x	1.88x slower → 0.81x
100	16.6 µs	6.4 µs	2.6x	2.04x slower → 0.90x
1000	226.3 µs	63.0 µs	3.6x	2.71x slower → 0.86x
10000	1751.5 µs	638.9 µs	2.7x	2.26x slower → 0.85x

RESP3 map parsing goes from consistently slower than the RESP2 equivalent to consistently faster. We found this while investigating a latency regression after redis-py 8 made RESP3 the default: HGETALL-heavy workloads got noticeably more expensive, and pinning protocol=2 recovered the throughput.

Implementation notes

• Pending keys live on a stack, not a single slot, because maps nest.
• The stack grows on demand rather than using a fixed bound. A container in value position is parented at creation time, so it never holds the stack deep; but a map in key position nests without limit. Such a reply always ends in TypeError for an unhashable key, and it must not be able to overflow a fixed buffer before getting there.
• Keys are owned references held across gets() calls, since a reply can be split over several feed() calls. They are therefore visited in Reader_traverse and released in Reader_dealloc.
• A reply aborted mid-map leaves keys whose values will never arrive, so Reader_gets drops the whole stack on its error path — not just the key in hand, since a reply nesting maps in key position keeps one buffered per level. That single site also covers the failures inside the map path itself (a failed push, or an unhashable key failing PyDict_SetItem): returning NULL is how a reply-object callback reports failure, so hiredis marks the reader as errored and stops, and the error path always runs.
• Growth is guarded against overflow in both the doubling and the byte count, following the same pattern as PushNotificationType_New.
• After a complete reply no key can still be waiting for its value. That is checked at runtime, not with assert: releases build with -DNDEBUG, where a push/pop imbalance would otherwise turn into a silent leak. It clears the stack and raises RuntimeError, matching the guard on the empty-stack case. Neither is reachable by construction; both are there so a future edit fails loudly.

Behavioural note

For a map with an unhashable key, the TypeError is now raised when the value is parsed rather than when the key is parsed. It is still raised out of the same gets() call. Two consequences, both limited to replies that were going to fail anyway:

• On input that is both malformed and contains an unhashable key, a different error may surface first (e.g. ProtocolError instead of TypeError).
• On input truncated between an unhashable key and its value, gets() now returns "not enough data" instead of raising, and raises once the value arrives. This seems the more defensible behaviour: the reply has not been fully read yet.

Testing

• Existing suite passes unchanged, including test_dict_memory_leaks under pytest --memray (50 KB limit) and test_gc.py.

• Ten new tests. Three are regression tests for the mis-pairing bug and fail on master:

  • test_dict_repeated_key_not_last
  • test_dict_repeated_key_interleaved
  • test_dict_hash_equal_keys

• Seven guard the paths this change introduces:

  • test_dict_split_between_key_and_value — key and value delivered in separate feed() calls.
  • test_dict_reused_reader — consecutive maps on one reader, each leaving a key buffered across a gets() that reports not-enough-data, locking in the cross-gets() key lifetime.
  • test_dict_protocol_error_after_key — reply aborted between a key and its value.
  • test_dict_pending_key_is_traversed — a buffered key is reported to the GC, so a cycle running through one stays collectable.
  • test_dict_protocol_error_releases_pending_key — the buffered key is released, checked with the reader still alive so it covers the release in gets() rather than the one in dealloc.
  • test_dict_deeply_nested_unhashable_keys — 12 levels of maps in key position, forcing the stack to grow past its initial capacity.
  • test_dict_deeply_nested_unhashable_keys_release — the same reply, asserting the whole stack is unwound: ten maps are still buffered when the failing pair is inserted, so releasing only the key in hand would strand the rest.

  The three tests that inspect the stack do so through gc.get_referents(reader), which reflects tp_traverse directly, so they assert on the exact object rather than on a memory-usage threshold. They are skipped off CPython, where get_referents does not report a C type's referents.

• No new warnings under -Wall -Wextra; the six reader.c emits are all pre-existing, in lines this PR does not touch.

Reproduction

import time
import hiredis


def resp3_map(n):
    parts = [b"%%%d\r\n" % n]
    for i in range(n):
        key, value = b"field%d" % i, b"value%d" % i
        parts.append(b"$%d\r\n%s\r\n$%d\r\n%s\r\n" % (len(key), key, len(value), value))
    return b"".join(parts)


def bench(payload, iters=200, runs=5):
    reader = hiredis.Reader()
    best = float("inf")
    for _ in range(runs):
        start = time.perf_counter()
        for _ in range(iters):
            reader.feed(payload)
            reader.gets()
        best = min(best, time.perf_counter() - start)
    return best / iters * 1e6


for size in (10, 100, 1000, 10000):
    print(f"{size:>6} fields: {bench(resp3_map(size)):8.1f} us")

---

Note

Medium Risk
Touches core reply parsing in the C extension (map assembly, error paths, GC traversal); behavior changes only for failing or edge-case map input, but correctness fixes affect all RESP3 map replies.

Overview
RESP3 maps are built by buffering each key on a growable reader-owned stack until its value arrives, then doing a single PyDict_SetItem—replacing the old Py_None placeholder + popitem() approach that mis-paired repeated or hash-equal keys and did extra work per entry.

The reader now owns pending keys across chunked feed()/gets(), visits them in tp_traverse, frees them in dealloc, clears the full stack on parse errors, and raises RuntimeError if any keys remain after a supposedly complete reply.

Tests add regressions for repeated/hash-equal keys, split feeds, protocol errors, deep nested maps, GC visibility of buffered keys, and reuse of one reader across multiple maps.

^{Reviewed by Cursor Bugbot for commit 7e9f011. Bugbot is set up for automated code reviews on this repo. Configure here.}

[uglide]

Hello @mmick78
Have you encountered this problem in real application usage, or is it a theoretical AI-generated fix?

[mmick78]

Hello @mmick78 Have you encountered this problem in real application usage, or is it a theoretical AI-generated fix?

hi @uglide, we did see in our production application a regression of performance when switching to redis-py 8.0.1 from previous version 6.4.0, which uses RESP3 by default instead of RESP2.
And we were seeing this issue only for certain commands, MGET for example was not impacted.

EDIT: So i checked again which regressions we faced, and it was only for HGETALL as mentioned in the PR description