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()
+classTestTokenize(unittest.TestCase):


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

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

Python/Python-tokenize.c

 }
 goto exit;
 }
 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 <pablogsal@gmail.com>

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 <lisandrosnik@gmail.com> Co-authored-by: Pablo Galindo <pablogsal@gmail.com>

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 <lisandrosnik@gmail.com> Co-authored-by: Pablo Galindo <pablogsal@gmail.com>

…hon#120318) Co-authored-by: Pablo Galindo <pablogsal@gmail.com>

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

bedevere-appbot mentioned this pull request Jun 10, 2024
Using internal tokenize module's TokenizerIter in multiple threads crashes #120317
Closed

bedevere-appbot 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 resolvedHide resolved

Use Py_BEGIN_CRITICAL_SECTION
abf568a

lysnikolaou force-pushed the tokenize-thread-safety branch from 78df829 to abf568aCompare 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 resolvedHide resolved
Python/Python-tokenize.cShow resolvedHide resolved
Python/Python-tokenize.c Outdated Show resolvedHide resolved

Fidget-Spinner reviewed Jun 10, 2024
View reviewed changes

Python/Python-tokenize.cShow resolvedHide resolved
Python/Python-tokenize.cShow resolvedHide resolved
Python/Python-tokenize.c Outdated Show resolvedHide resolved
Python/Python-tokenize.cShow resolvedHide resolved

Address feedback & improve test
814d9af

Fidget-Spinner approved these changes Jun 10, 2024
View reviewed changes

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

bedevere-appbot 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.pyShow resolvedHide 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 moduleJun 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.cShow resolvedHide 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 1c51dafCompare June 11, 2024 20:34

Lock around all of tokenizeriter_next
ca466fa
Co-authored-by: Pablo Galindo <pablogsal@gmail.com>

lysnikolaou force-pushed the tokenize-thread-safety branch from 1c51daf to ca466faCompare 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.cShow resolvedHide 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:mainJul 16, 2024

bedevere-appbot removed the awaiting merge label Jul 16, 2024

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

bedevere-appbot 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 <pablogsal@gmail.com>



		@threading_helper.requires_working_threading()
		classTestTokenize(unittest.TestCase):

Uh oh!

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

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

Uh oh!

Conversation

lysnikolaou commented Jun 10, 2024• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner commented Jun 10, 2024

Uh oh!

lysnikolaou commented Jun 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lysnikolaou commented Jun 10, 2024

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lysnikolaou commented Jun 10, 2024

Uh oh!

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsalJun 11, 2024

Choose a reason for hiding this comment

Uh oh!

pablogsalJun 11, 2024

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Jun 11, 2024

Uh oh!

pablogsal commented Jun 11, 2024

Uh oh!

lysnikolaou commented Jun 11, 2024

Uh oh!

Uh oh!

colesburyJun 27, 2024

Choose a reason for hiding this comment

Uh oh!

lysnikolaouJul 1, 2024

Choose a reason for hiding this comment

Uh oh!

lysnikolaou commented Jul 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-appbot commented Jul 16, 2024

Uh oh!

bedevere-appbot commented Jul 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

lysnikolaou commented Jul 1, 2024•
edited
Loading