Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs/advanced A document about deadlock potential with C++ statics #5394

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

tkoeppe
Copy link
Contributor

@tkoeppe tkoeppe commented Oct 2, 2024

Description

A detailed look at the interaction of the Python GIL and C++ static variable initializer guard mutexes, and in particular its potential for deadlocks.

Suggested changelog entry:

A new "Double locking, deadlocking, GIL" document was added. (Currently it does not appear in the readthedocs view. Help welcome.)

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Oct 2, 2024

@rwgk Please have a look.

@rwgk
Copy link
Collaborator

rwgk commented Oct 2, 2024

Awesome, thanks @tkoeppe!

Formatting of the tables doesn't work in my view (Files Changes -> ... -> View file).

Before trying to fix up the .md format to make the GitHub renderer happy, let's ask @henryiii: Is it OK to keep this as an .md file? Or are there strong reasons to convert to .rst?

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Oct 2, 2024

@rwgk Yes, I saw about the tables -- is there any alternative, supported table syntax? (But indeed, let's first see about RST.)

@henryiii
Copy link
Collaborator

henryiii commented Oct 3, 2024

I'd love to have more .md, and I'll be moving the changelog to md in the future.

@henryiii
Copy link
Collaborator

henryiii commented Oct 3, 2024

|     | T1                  | T2                  |
|-----|---------------------|---------------------|
| 1   | `mu1.lock()`{.good} | `mu2.lock()`{.good} |
| 2   | `mu2.lock()`{.bad}  | `mu1.lock()`{.bad}  |
| 3   | `/* work */`        | `/* work */`        |
| 4   | `mu2.unlock()`      | `mu1.unlock()`      |
| 5   | `mu1.unlock()`      | `mu2.unlock()`      |
T1 T2
1 mu1.lock(){.good} mu2.lock(){.good}
2 mu2.lock(){.bad} mu1.lock(){.bad}
3 /* work */ /* work */
4 mu2.unlock() mu1.unlock()
5 mu1.unlock() mu2.unlock()

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Oct 3, 2024

Thanks! Will apply.

Any suggesions on how we can style "good"/"bad" examples (as green/red)?

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Oct 3, 2024

(Also, while I'm sending individual commits, probably best to squash when merging.)

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

More work for LATER (I'm giving up after spending about an hour in vain):

  • [TOC], {.good} and .{bad} don't render as hoped,

  • the cross-references are not clickable, and

  • the new doc isn't included in the readthedocs side panel.

But most importantly, the content is complete and highly informative.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Oct 8, 2024

We can get rid of {.good} etc. Those are meant to style the code (in red/green), but it's not necessary.

@rwgk
Copy link
Collaborator

rwgk commented Oct 8, 2024

We can get rid of {.good} etc. Those are meant to style the code (in red/green), but it's not necessary.

I find them useful when reading the doc, although it doesn't look fancy.

@rwgk rwgk merged commit af67e87 into pybind:master Oct 8, 2024
79 of 80 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants