aio-libs / multidict / 37054227922

86%

Build Type

push

github

Committed by web-flow

Commit Message

Hold a compact key before reading its hash in lock-free lookups (#1653)

<!-- Thank you for your contribution! -->

## What do these changes do?

Fixes the ThreadSanitizer job, which has failed on master since #1648. A
compact table entry keeps no hash, so a lock-free `get()`, `d[key]` or
`in` on the free-threaded build read it through the stored key. A
concurrent delete could already have freed that key and the allocator
reused its block for a new object;
`test_ci_lookups_race_deletes_that_free_keys` hits this (master push
runs 36992438938 and 37005918410). The CIMultiDict path also read
`istr->canonical` through the freed key, and a MultiDict of plain `str`
keys has the same bug.

The lookup now takes a validated reference to a compact key before
reading anything through it, as CPython's own dict lookup does on its
unicode-only tables, except when the stored key is the probe's own
object, whose address the caller's reference keeps from being reused.
`try_get_ref()` takes CPython's owner-thread and immortal fast path
inline rather than always calling `PyUnstable_TryIncRef()`. The full
layout and the GIL build are unchanged.

The extra reference on colliding slots costs FT lookups a little
(callgrind, against master at 43ef7fdc; the GIL build is bit-identical):

| operation | class | before | after | delta |
| --- | --- | --- | --- | --- |
| `d.get(key)`, miss | MultiDict | 333.9 | 365.3 | +9.4% |
| `d[key]` | MultiDict | 197.3 | 206.8 | +4.8% |
| `key in d` | MultiDict | 164.1 | 170.1 | +3.7% |
| `d[key]`, `istr` key | CIMultiDict | 204.3 | 211.8 | +3.7% |
| `d[key] = v`, new key | CIMultiDict | 647.7 | 635.7 | -1.9% |

Deferring the key decrefs until no reader is active would have kept
reads free, but with readers always in flight the deferred keys could
grow without bound, so that was not taken.

## Are there changes in behavior for the user?

No, apart from the fixed race.

## Is it a substantial burden for the maintainers to support this?

No; th... (continued)

Coverage Stats

791 of 1582 branches covered (50.0%)

Branch coverage included in aggregate %.

12 of 24 new or added lines in 1 file covered. (50.0%)

2 existing lines in 1 file now uncovered.

9139 of 9971 relevant lines covered (91.66%)

1.83 hits per line