Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES/1328.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fixed race when calling for a string representation of a ``MultiDict`` object.
Comment thread
Vizonex marked this conversation as resolved.
Outdated
-- by :user:`Vizonex`
Comment thread
Vizonex marked this conversation as resolved.
Comment thread
Vizonex marked this conversation as resolved.
2 changes: 2 additions & 0 deletions multidict/_multilib/hashtable.h
Original file line number Diff line number Diff line change
Expand Up @@ -1769,6 +1769,7 @@ md_repr(MultiDictObject *md, PyObject *name, bool show_keys, bool show_values)
PyObject *key = NULL;
PyObject *value = NULL;

Py_BEGIN_CRITICAL_SECTION(md);
bool comma = false;
uint64_t version = md->version;

Expand Down Expand Up @@ -1850,6 +1851,7 @@ md_repr(MultiDictObject *md, PyObject *name, bool show_keys, bool show_values)
Py_CLEAR(key);
Py_CLEAR(value);
PyUnicodeWriter_Discard(writer);
Py_END_CRITICAL_SECTION();
return NULL;
}

Expand Down
37 changes: 37 additions & 0 deletions tests/isolated/multidict_repr_ft.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import os
import subprocess
import sys
import sysconfig
import textwrap

FREETHREADED = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be moved to the skipif mark in the pytest param.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do also since we seem to have a ton of bugs to patch. I was thinking about a new module called test_free_threaded.py when #1327 is merged to just have both of them in a new file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if @Dreamsorcerer and @bdraco have opinions on this, then.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely the tests should pass on regular versions of Python?



if __name__ == "__main__" and FREETHREADED:
child = textwrap.dedent("""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also get rid of the double-subprocess indirectly here. If we want to invoke a bunch of Python code with different Python flags, we can put those flags into the test parametrization. Additionally, a chunk of Python in a string is not lintable, and the coverage is not getting measured. It should definitely live in a Python module as a first-class citizen.

cc @Dreamsorcerer

import signal, threading, time
signal.alarm(20)
from multidict import MultiDict
md = MultiDict([(f"k{i}", i) for i in range(50)])
errors = []
stop = threading.Event()
def repr_loop():
while not stop.is_set():
try: repr(md)
except Exception as e: errors.append(type(e).__name__)
def mutate_loop():
i = 0
while not stop.is_set(): md.add(f"m{i}", i); i += 1
ts = [threading.Thread(target=repr_loop, daemon=True) for _ in range(2)]
ts += [threading.Thread(target=mutate_loop, daemon=True) for _ in range(2)]
for t in ts: t.start()
time.sleep(0.3); stop.set(); time.sleep(0.05)
print(f"repr errors: {len(errors)}")
""")
subprocess.run(
[sys.executable, "-c", child],
env={**os.environ, "PYTHON_GIL": "0"},
capture_output=True,
timeout=60,
check=True,
)
1 change: 1 addition & 0 deletions tests/test_leaks.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
@pytest.mark.parametrize(
("script"),
(
"multidict_repr_ft.py",
"multidict_extend_dict.py",
"multidict_extend_multidict.py",
"multidict_extend_tuple.py",
Expand Down
Loading