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

Delay releasing of fiber stack in multithread mode #8138

Merged
merged 1 commit into from
Sep 2, 2019

Conversation

waj
Copy link
Member

@waj waj commented Sep 2, 2019

This PR fixes an issue found by @asterite in multithread mode (-Dpreview_mt)
For example, this code currently fails with really random and nasty errors:

1000000.times { spawn { } }
sleep 0.1

This is because the fiber stacks are reused but in multithread mode this reuse can happen before the current fiber actually finishes using it.

I didn't add a spec because I don't know how to consistently reproduce it with an example that doesn't involve 1M fibers :)

Once the stack is released it can be used by any other fiber. This patch
ensures the release is done only after the current fiber has finished executing.

In single thread mode this change is not needed because there cannot be new fibers
until the context is switched to another fiber.
@waj waj added the kind:bug label Sep 2, 2019
@ysbaddaden
Copy link
Contributor

Can the queue grow quite big before the scheduler can consider releasing a stack (if the event queue is kept full).

Should removing the fiber from the linked list be delayed, too? A GC collection could start after it's removed, yet before the context switch. Maybe not much impactful thought.

@waj
Copy link
Member Author

waj commented Sep 2, 2019

The queue only grows when the call to reschedule ends resuming a new fiber, that is, a fiber that has never called reschedule before. Otherwise the resuming fiber will be waiting just within the call to reschedule and it will release the pending stacks before resuming its execution.
We could add a call to clear pending stacks right before a new fiber is started, but in my tests I didn't notice much difference.

Regarding the linked list and the GC, your concern sounds reasonable, but I didn't notice any issue so far. I'll take it into account and address that issue in a different PR if needed.

@waj waj merged commit 43b36f6 into crystal-lang:master Sep 2, 2019
@bcardiff bcardiff added this to the 0.31.0 milestone Sep 3, 2019
@waj waj deleted the fix/mt-stack-release branch February 12, 2020 20:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants