GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-120317: Lock around global state in the tokenize module by lysnikolaou · Pull Request #120318 · python/cpython

Conversation

@lysnikolaou

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Using internal tokenize module's TokenizerIter in multiple threads crashes #120317

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lysnikolaou
pythongh-120317: Lock around tokenizer calls in the tokenizer module
900dd6a
@bedevere-app bedevere-app Bot mentioned this pull request Jun 10, 2024
Using internal tokenize module's TokenizerIter in multiple threads crashes #120317
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 10, 2024
@lysnikolaou lysnikolaou added the skip news label Jun 10, 2024
@lysnikolaou
lysnikolaou requested a review from pablogsal June 10, 2024 13:31
Fidget-Spinner
Fidget-Spinner reviewed Jun 10, 2024
View reviewed changes
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou
Use Py_BEGIN_CRITICAL_SECTION
abf568a
@lysnikolaou
lysnikolaou force-pushed the tokenize-thread-safety branch from 78df829 to abf568a Compare June 10, 2024 13:50
@Fidget-Spinner

Fidget-Spinner commented Jun 10, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lysnikolaou added 2 commits June 10, 2024 17:02
@lysnikolaou
Add more locking around global tokenizer state
9dfe174
@lysnikolaou
Add test
5fd07b6
@lysnikolaou

lysnikolaou commented Jun 10, 2024

Copy link
Copy Markdown
Member Author

@Fidget-Spinner I added more locking around global state. Reviewing each commit seprately might make it easier.
Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Jun 10, 2024
View reviewed changes
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Jun 10, 2024
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This looks like a refactor, so good so far.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou
Address feedback & improve test
814d9af
@lysnikolaou

lysnikolaou commented Jun 10, 2024

Copy link
Copy Markdown
Member Author

Feedback addressed.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner approved these changes Jun 10, 2024
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jun 10, 2024
@lysnikolaou
Guard atomic store behind Py_GIL_DISABLED
c1093be
Fidget-Spinner
Fidget-Spinner reviewed Jun 10, 2024
View reviewed changes
Comment thread Lib/test/test_free_threading/test_tokenize.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou
Add requires working threading decorator to test
117d256
@lysnikolaou

lysnikolaou commented Jun 10, 2024

Copy link
Copy Markdown
Member Author

All feedback addressed. Thanks a lot for the reviews and the help @Fidget-Spinner and @erlend-aasland!
erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou 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
Fidget-Spinner approved these changes Jun 10, 2024
View reviewed changes
@lysnikolaou
Merge branch 'main' into tokenize-thread-safety
975f01a
pablogsal
pablogsal reviewed Jun 11, 2024
View reviewed changes
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal approved these changes Jun 11, 2024
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pablogsal
pablogsal reviewed Jun 11, 2024
View reviewed changes
Comment thread Lib/test/test_free_threading/test_tokenize.py


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

@pablogsal pablogsal Jun 11, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lysnikolaou reacted with laugh emoji
All reactions
  • 😄 1 reaction
pablogsal
pablogsal reviewed Jun 11, 2024
View reviewed changes
Comment thread Python/Python-tokenize.c
@@ -194,7 +252,13 @@ tokenizeriter_next(tokenizeriterobject *it)
}
if (it->done || type == ERRORTOKEN) {

@pablogsal pablogsal Jun 11, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pablogsal

pablogsal commented Jun 11, 2024

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal

pablogsal commented Jun 11, 2024

Copy link
Copy Markdown
Member

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;
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou
lysnikolaou force-pushed the tokenize-thread-safety branch 2 times, most recently from 20c7cb2 to 1c51daf Compare June 11, 2024 20:34
@lysnikolaou @pablogsal
Lock around all of tokenizeriter_next
ca466fa 
Co-authored-by: Pablo Galindo <[email protected]>
@lysnikolaou
lysnikolaou force-pushed the tokenize-thread-safety branch from 1c51daf to ca466fa Compare June 11, 2024 20:34
@lysnikolaou

lysnikolaou commented Jun 11, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou lysnikolaou added the needs backport to 3.13 only security fixes label Jun 11, 2024
Fidget-Spinner
Fidget-Spinner reviewed Jun 12, 2024
View reviewed changes
Comment thread Python/Python-tokenize.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou lysnikolaou added the topic-free-threading label Jun 12, 2024
lysnikolaou added 2 commits June 12, 2024 20:59
@lysnikolaou
Merge branch 'main' into tokenize-thread-safety
3abb8ab
@lysnikolaou
Fix formatting
601e539
@erlend-aasland erlend-aasland mentioned this pull request Jun 17, 2024
gh-120608: Make reversed thread-safe #120609
Closed
colesbury
colesbury reviewed Jun 27, 2024
View reviewed changes
Comment thread Lib/test/test_free_threading/test_tokenize.py Outdated
try:
r = next(it)
tokens.append(tokenize.TokenInfo._make(r))
time.sleep(1)

@colesbury colesbury Jun 27, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@lysnikolaou lysnikolaou Jul 1, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lysnikolaou added 2 commits July 1, 2024 10:52
@lysnikolaou
Merge branch 'main' into tokenize-thread-safety
6290f19
@lysnikolaou
Change sleeping time
0ddde2f
@lysnikolaou

lysnikolaou commented Jul 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal approved these changes Jul 13, 2024
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Reviewed the PR and the comments and it LGTM

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
erlend-aasland
erlend-aasland approved these changes Jul 16, 2024
View reviewed changes
@lysnikolaou
lysnikolaou merged commit 8549559 into python:main Jul 16, 2024
@miss-islington-app

miss-islington-app Bot commented Jul 16, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Jul 16, 2024
@lysnikolaou
lysnikolaou deleted the tokenize-thread-safety branch July 16, 2024 09:36
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 16, 2024
@lysnikolaou @pablogsal @miss-islington
pythongh-120317: Lock around global state in the tokenize module (pyt…
52db480 
…honGH-120318)

(cherry picked from commit 8549559)

Co-authored-by: Lysandros Nikolaou <[email protected]>
Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-app

bedevere-app Bot commented Jul 16, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jul 16, 2024
lysnikolaou added a commit that referenced this pull request Jul 16, 2024
@miss-islington @lysnikolaou @pablogsal
[3.13] gh-120317: Lock around global state in the tokenize module (GH…
e45c61f 
…-120318) (#121841)

(cherry picked from commit 8549559)

Co-authored-by: Lysandros Nikolaou <[email protected]>
Co-authored-by: Pablo Galindo <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@lysnikolaou @pablogsal @estyxx
pythongh-120317: Lock around global state in the tokenize module (pyt…
140bb6c 
…hon#120318)

Co-authored-by: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@pablogsal pablogsal pablogsal approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@lysnikolaou @Fidget-Spinner @pablogsal @colesbury @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.