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

Fix Reference#exec_recursive, #exec_recursive_clone to be fiber aware #15361

Conversation

ysbaddaden
Copy link
Contributor

Recursion of the #inspect and #pretty_print methods is handled by a global hash, which is a per-thread global, but if any of these methods yield —which will happen because they're writing to an IO— another fiber running on the same thread will falsely detect a recursion if it inspects the same object.

This patch moves Reference#exec_recursive methods as instances of Fiber, using a per-fiber hash instead of per-thread/process hash.

The example from #15088 now correctly prints:

0: [YieldInspect]
1: [YieldInspect]

Recursion of the `#inspect` and `#pretty_print` methods is handled by a
global hash, which is a per-thread global, but if any of these methods
yield —which will happen because they're writing to an IO— another
fiber running on the same thread will falsely detect a recursion if it
inspects the same object.

This patch moves `Reference#exec_recursive` methods as instances of
`Fiber`, using a per-fiber hash instead of per-thread/process hash.

References crystal-lang#15088
@ysbaddaden ysbaddaden added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:concurrency labels Jan 21, 2025
@ysbaddaden ysbaddaden self-assigned this Jan 21, 2025
@ysbaddaden ysbaddaden force-pushed the refactor/exec-recursive-isnt-fiber-safe branch from b8ab18c to b845167 Compare January 21, 2025 16:37
@ysbaddaden
Copy link
Contributor Author

Thinking again about this: there's no need to move the implementation to Fiber, we only need to move the hash there.

@RX14
Copy link
Contributor

RX14 commented Jan 22, 2025

This PR is OK with me, but is there anything covering the fiber-local variable usecase which is doable outside the stdlib?

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 23, 2025

@RX14 Nothing generic. We should open a new issue about that, it would be interesting to have something like a macro or annotation to keep fiber local data, without reopening Fiber and forever expanding its size.

@straight-shoota straight-shoota merged commit df06679 into crystal-lang:master Jan 23, 2025
70 checks passed
@ysbaddaden ysbaddaden deleted the refactor/exec-recursive-isnt-fiber-safe branch January 23, 2025 16:49
@straight-shoota straight-shoota changed the title Reference#exec_recursive[_clone] aren't fiber aware Fix Reference#exec_recursive, #exec_recursive_clone to be fiber aware Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:concurrency
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants