gh-120317: Lock around global state in the tokenize module #120318

lysnikolaou · 2024-06-10T13:30:55Z

Issue: Using internal tokenize module's TokenizerIter in multiple threads crashes #120317

Python/Python-tokenize.c

Fidget-Spinner · 2024-06-10T14:26:20Z

This LGTM. Can you add the reproducer to test.test_free_threading please? I'll approve after!

lysnikolaou · 2024-06-10T15:26:55Z

@Fidget-Spinner I added more locking around global state. Reviewing each commit seprately might make it easier.

Python/Python-tokenize.c

Fidget-Spinner

This looks like a refactor, so good so far.

Python/Python-tokenize.c

lysnikolaou · 2024-06-10T16:06:42Z

Feedback addressed.

@Fidget-Spinner I used _Py_atomic_store_int instead of the macro. Is that okay?

Fidget-Spinner

LGTM. Feel free to ignore the following comments if this part of the tokenizer is not perf sensitive.

Python/Python-tokenize.c

Lib/test/test_free_threading/test_tokenize.py

lysnikolaou · 2024-06-10T16:25:57Z

All feedback addressed. Thanks a lot for the reviews and the help @Fidget-Spinner and @erlend-aasland!

Python/Python-tokenize.c

pablogsal

LGTM

pablogsal · 2024-06-11T17:23:53Z

Lib/test/test_free_threading/test_tokenize.py

+
+
+@threading_helper.requires_working_threading()
+class TestTokenize(unittest.TestCase):


Tested this locally: it crashes spectacularly without the fix :)

pablogsal · 2024-06-11T17:25:02Z

Python/Python-tokenize.c

@@ -194,7 +252,13 @@ tokenizeriter_next(tokenizeriterobject *it)
 }
 if (it->done || type == ERRORTOKEN) {


Don't we need to protect the read? Otherwise we may get weird partial reads no?

pablogsal · 2024-06-11T17:26:17Z

Actually, one question here: shouldn't we be protecting all reads of the it structure that are not under critical sections? Otherwise we are getting some partial reads no? I am missing something?

pablogsal · 2024-06-11T17:27:10Z

For example, this read is not protected, no?:

    Py_ssize_t lineno = ISSTRINGLIT(type) ? it->tok->first_lineno : it->tok->lineno;
     Py_ssize_t end_lineno = it->tok->lineno;

Co-authored-by: Pablo Galindo <[email protected]>

lysnikolaou · 2024-06-11T20:35:34Z

After some offline discussion with @pablogsal, we decided it makes more sense to just lock around all of tokenizeriter_next.

Python/Python-tokenize.c

colesbury · 2024-06-27T18:14:42Z

Lib/test/test_free_threading/test_tokenize.py

+ try:
+ r = next(it)
+ tokens.append(tokenize.TokenInfo._make(r))
+ time.sleep(1)


Can we make this test faster? It's going to run on every CI job. I think we should aim for 0.1 seconds or less for these sorts of tests. (It's currently ~3 seconds).

Changed the amount of time each thread sleeps to 0.03 secs, which means that the whole test now takes about 0.1 seconds. Also verified that the test still fails without the fix and succeeds with it.

lysnikolaou · 2024-07-01T12:39:27Z

@pablogsal Can you do a final review here when you have some spare cycles?

pablogsal

Reviewed the PR and the comments and it LGTM

I also ran the tests locally in free threaded mode to confirm

miss-islington-app · 2024-07-16T09:36:01Z

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

…honGH-120318) (cherry picked from commit 8549559) Co-authored-by: Lysandros Nikolaou <[email protected]> Co-authored-by: Pablo Galindo <[email protected]>

bedevere-app · 2024-07-16T09:36:14Z

GH-121841 is a backport of this pull request to the 3.13 branch.

…-120318) (#121841) (cherry picked from commit 8549559) Co-authored-by: Lysandros Nikolaou <[email protected]> Co-authored-by: Pablo Galindo <[email protected]>

…hon#120318) Co-authored-by: Pablo Galindo <[email protected]>

pythongh-120317: Lock around tokenizer calls in the tokenizer module

900dd6a

bedevere-app bot mentioned this pull request Jun 10, 2024

Using internal tokenize module's TokenizerIter in multiple threads crashes #120317

Closed

bedevere-app bot added the awaiting core review label Jun 10, 2024

lysnikolaou added the skip news label Jun 10, 2024

lysnikolaou requested a review from pablogsal June 10, 2024 13:31

Fidget-Spinner reviewed Jun 10, 2024

View reviewed changes

Python/Python-tokenize.c Outdated Show resolved Hide resolved

Use Py_BEGIN_CRITICAL_SECTION

abf568a

lysnikolaou force-pushed the tokenize-thread-safety branch from 78df829 to abf568a Compare June 10, 2024 13:50

lysnikolaou added 2 commits June 10, 2024 17:02

Add more locking around global tokenizer state

9dfe174

Add test

5fd07b6

erlend-aasland reviewed Jun 10, 2024

View reviewed changes

Python/Python-tokenize.c Outdated Show resolved Hide resolved

Python/Python-tokenize.c Show resolved Hide resolved

Python/Python-tokenize.c Outdated Show resolved Hide resolved

Fidget-Spinner reviewed Jun 10, 2024

View reviewed changes

Python/Python-tokenize.c Show resolved Hide resolved

Python/Python-tokenize.c Show resolved Hide resolved

Python/Python-tokenize.c Outdated Show resolved Hide resolved

Python/Python-tokenize.c Show resolved Hide resolved

Address feedback & improve test

814d9af

Fidget-Spinner approved these changes Jun 10, 2024

View reviewed changes

Python/Python-tokenize.c Outdated Show resolved Hide resolved

Python/Python-tokenize.c Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting core review labels Jun 10, 2024

Guard atomic store behind Py_GIL_DISABLED

c1093be

Fidget-Spinner reviewed Jun 10, 2024

View reviewed changes

Lib/test/test_free_threading/test_tokenize.py Show resolved Hide resolved

Add requires working threading decorator to test

117d256

lysnikolaou changed the title ~~gh-120317: Lock around tokenizer calls in the tokenizer module~~ gh-120317: Lock around global state in the tokenize module Jun 10, 2024

Fidget-Spinner approved these changes Jun 10, 2024

View reviewed changes

Merge branch 'main' into tokenize-thread-safety

975f01a

pablogsal reviewed Jun 11, 2024

View reviewed changes

Python/Python-tokenize.c Outdated Show resolved Hide resolved

pablogsal approved these changes Jun 11, 2024

View reviewed changes

pablogsal reviewed Jun 11, 2024

View reviewed changes

lysnikolaou force-pushed the tokenize-thread-safety branch 2 times, most recently from 20c7cb2 to 1c51daf Compare June 11, 2024 20:34

Lock around all of tokenizeriter_next

ca466fa

Co-authored-by: Pablo Galindo <[email protected]>

lysnikolaou force-pushed the tokenize-thread-safety branch from 1c51daf to ca466fa Compare June 11, 2024 20:34

lysnikolaou added the needs backport to 3.13 bugs and security fixes label Jun 11, 2024

Fidget-Spinner reviewed Jun 12, 2024

View reviewed changes

Python/Python-tokenize.c Show resolved Hide resolved

lysnikolaou added the topic-free-threading label Jun 12, 2024

lysnikolaou added 2 commits June 12, 2024 20:59

Merge branch 'main' into tokenize-thread-safety

3abb8ab

Fix formatting

601e539

erlend-aasland mentioned this pull request Jun 17, 2024

gh-120608: Make reversed thread-safe #120609

Closed

colesbury reviewed Jun 27, 2024

View reviewed changes

lysnikolaou added 2 commits July 1, 2024 10:52

Merge branch 'main' into tokenize-thread-safety

6290f19

Change sleeping time

0ddde2f

pablogsal approved these changes Jul 13, 2024

View reviewed changes

erlend-aasland approved these changes Jul 16, 2024

View reviewed changes

lysnikolaou merged commit 8549559 into python:main Jul 16, 2024
34 checks passed

bedevere-app bot removed the awaiting merge label Jul 16, 2024

lysnikolaou deleted the tokenize-thread-safety branch July 16, 2024 09:36

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 16, 2024

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024

pythongh-120317: Lock around global state in the tokenize module (pyt…

140bb6c

…hon#120318) Co-authored-by: Pablo Galindo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-120317: Lock around global state in the tokenize module #120318

gh-120317: Lock around global state in the tokenize module #120318

lysnikolaou commented Jun 10, 2024 •

edited by bedevere-app bot

Loading

Fidget-Spinner commented Jun 10, 2024

lysnikolaou commented Jun 10, 2024

Fidget-Spinner left a comment

lysnikolaou commented Jun 10, 2024

Fidget-Spinner left a comment

lysnikolaou commented Jun 10, 2024

pablogsal left a comment

pablogsal Jun 11, 2024

pablogsal Jun 11, 2024

pablogsal commented Jun 11, 2024

pablogsal commented Jun 11, 2024

lysnikolaou commented Jun 11, 2024

colesbury Jun 27, 2024

lysnikolaou Jul 1, 2024

lysnikolaou commented Jul 1, 2024 •

edited

Loading

pablogsal left a comment

miss-islington-app bot commented Jul 16, 2024

bedevere-app bot commented Jul 16, 2024



		@threading_helper.requires_working_threading()
		class TestTokenize(unittest.TestCase):

		@@ -194,7 +252,13 @@ tokenizeriter_next(tokenizeriterobject *it)
		}
		if (it->done \|\| type == ERRORTOKEN) {

gh-120317: Lock around global state in the tokenize module #120318

gh-120317: Lock around global state in the tokenize module #120318

Conversation

lysnikolaou commented Jun 10, 2024 • edited by bedevere-app bot Loading

Fidget-Spinner commented Jun 10, 2024

lysnikolaou commented Jun 10, 2024

Fidget-Spinner left a comment

Choose a reason for hiding this comment

lysnikolaou commented Jun 10, 2024

Fidget-Spinner left a comment

Choose a reason for hiding this comment

lysnikolaou commented Jun 10, 2024

pablogsal left a comment

Choose a reason for hiding this comment

pablogsal Jun 11, 2024

Choose a reason for hiding this comment

pablogsal Jun 11, 2024

Choose a reason for hiding this comment

pablogsal commented Jun 11, 2024

pablogsal commented Jun 11, 2024

lysnikolaou commented Jun 11, 2024

colesbury Jun 27, 2024

Choose a reason for hiding this comment

lysnikolaou Jul 1, 2024

Choose a reason for hiding this comment

lysnikolaou commented Jul 1, 2024 • edited Loading

pablogsal left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Jul 16, 2024

bedevere-app bot commented Jul 16, 2024

lysnikolaou commented Jun 10, 2024 •

edited by bedevere-app bot

Loading

lysnikolaou commented Jul 1, 2024 •

edited

Loading