Skip to content

Introduce RUBY_ASSERT(ruby_thread_has_gvl_p()) to detect invalid interface usage. #11975

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 14, 2025
Merged

Introduce RUBY_ASSERT(ruby_thread_has_gvl_p()) to detect invalid interface usage. #11975

merged 3 commits into from
Apr 14, 2025

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Nov 1, 2024 •
edited
Loading

https://bugs.ruby-lang.org/issues/20877

This was originally to demonstrate/show the problems with zlib. That has now been fixed: ruby/zlib#88

This change helps us find other (similar) issues and prevent regressions.

@ioquatix ioquatix requested a review from jeremyevans November 1, 2024 03:06
jeremyevans

This comment was marked as outdated.

Sign in to view

@jeremyevans

This comment was marked as outdated.

Sign in to view
@ioquatix ioquatix marked this pull request as draft November 7, 2024 00:28
@ioquatix ioquatix force-pushed the assert-gvl branch 2 times, most recently from 4258ece to 2407017 Compare November 7, 2024 00:30
jeremyevans
jeremyevans approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

I recommend updating the commit message to describe that this is being done to debug issues in the zlib extension. Code changes look fine.

@ioquatix ioquatix marked this pull request as ready for review November 7, 2024 01:01
@ioquatix
Copy link
Member Author

ioquatix commented Nov 7, 2024

The reason why this PR is failing is because zlib.c is using the C functions without holding the GVL. Once ruby/zlib#88 is merged, this PR will become green.

@ioquatix ioquatix force-pushed the assert-gvl branch 8 times, most recently from 5c67610 to d129a25 Compare November 20, 2024 22:26
@ioquatix ioquatix requested review from ko1 and nobu November 20, 2024 22:28
@ioquatix ioquatix force-pushed the assert-gvl branch 3 times, most recently from feb1785 to 21298de Compare November 20, 2024 22:46
@ioquatix ioquatix requested a review from eregon November 20, 2024 22:46
@ioquatix ioquatix force-pushed the assert-gvl branch 3 times, most recently from fed65e8 to 44d74e1 Compare November 21, 2024 00:49
ko1
ko1 reviewed Nov 21, 2024
View reviewed changes
@ko1
Copy link
Contributor

ko1 commented Nov 21, 2024

Do you plan to introduce assertions to all of rb_ prefix C API?

@ioquatix
Copy link
Member Author

Do you plan to introduce assertions to all of rb_ prefix C API?

I feel like we should, but it's also quite a lot of work. So for now, my main goal is to find any bugs. Therefore, adding it to methods like rb_xxx_modify and so on may be sufficient.

If you have a different idea, please let me know.

@ioquatix ioquatix force-pushed the assert-gvl branch 2 times, most recently from 83b7e0a to c6e76ce Compare November 21, 2024 11:38
@ioquatix ioquatix requested a review from ko1 November 22, 2024 01:11
@launchable-app Launchable app

This comment has been minimized.

Sign in to view
@ioquatix ioquatix force-pushed the assert-gvl branch 2 times, most recently from b144c5c to 53b79bc Compare April 14, 2025 07:19
@ioquatix ioquatix enabled auto-merge (squash) April 14, 2025 07:19
@ioquatix ioquatix disabled auto-merge April 14, 2025 07:46
@ioquatix ioquatix enabled auto-merge (rebase) April 14, 2025 07:46
@ioquatix ioquatix merged commit c13ac4d into ruby:master Apr 14, 2025
80 checks passed
@ioquatix ioquatix deleted the assert-gvl branch April 15, 2025 09:51
@samuel-williams-shopify
Copy link

If you see an error like this:

string.c:3660: Assertion Failed: rb_str_set_len:ruby_thread_has_gvl_p()

It might mean you are (1) using a build of Ruby that includes this PR, but (2) haven't updated zlib gem to the latest version which does not trigger this assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyevans jeremyevans jeremyevans approved these changes

@nobu nobu Awaiting requested review from nobu

@eregon eregon Awaiting requested review from eregon

@ko1 ko1 Awaiting requested review from ko1

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ioquatix @jeremyevans @ko1 @samuel-williams-shopify