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

calling `Fiber#resume` directly from C causes error #3056

Closed
kazuho opened this Issue Dec 23, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@kazuho
Contributor

kazuho commented Dec 23, 2015

When Fiber#resume is called directly from C, the method returns the argument passed, without running any code on the fiber context.

Looking at the source code, it seems like that the fiber implementation is not handling such case.

In the current implementation, fiber_switch (called by mrb_fiber_resume) replaces the content (i.e. mrb->c) and returns, hoping that the VM will continue running the operations on the changed context. This works fine if mrb_context_run is the caller of mrb_fiber_resume.

However in case Fiber#resume is called directly from C code, mrb_fiber_resume returns directly to mrb_funcall_with_block and then to the C code, without running the operations on the changed context.

Originally discussed in this tweet. Sorry for the lack of code for reproducing the issue.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Dec 23, 2015

Member

Can you show us your code snippet? It doesn't have to be reproducible.
I just want to know how you called #resume.

Member

matz commented Dec 23, 2015

Can you show us your code snippet? It doesn't have to be reproducible.
I just want to know how you called #resume.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 24, 2015

Contributor

@matz Thank you for looking into the issue.

The diff is h2o/h2o@3152e4c. The commit removes the wrapper used to call Fiber#resume, and calls the method directly.

With the change, the mruby handler of H2O stops working correctly; when called for the first time, it returns the argument passed to Fiber#resume, succeeding calls end up in exceptions that say: FiberError: can't cross C function boundary.

Contributor

kazuho commented Dec 24, 2015

@matz Thank you for looking into the issue.

The diff is h2o/h2o@3152e4c. The commit removes the wrapper used to call Fiber#resume, and calls the method directly.

With the change, the mruby handler of H2O stops working correctly; when called for the first time, it returns the argument passed to Fiber#resume, succeeding calls end up in exceptions that say: FiberError: can't cross C function boundary.

@Asmod4n

This comment has been minimized.

Show comment
Hide comment
@Asmod4n

Asmod4n Dec 24, 2015

Contributor

Have you tried mrb_fiber_resume from https://github.com/mruby/mruby/blob/master/mrbgems/mruby-fiber/src/fiber.c#L231 yet? fiber_switch protects from calling Fiber#resume via mrb_funcall: https://github.com/mruby/mruby/blob/master/mrbgems/mruby-fiber/src/fiber.c#L166

Contributor

Asmod4n commented Dec 24, 2015

Have you tried mrb_fiber_resume from https://github.com/mruby/mruby/blob/master/mrbgems/mruby-fiber/src/fiber.c#L231 yet? fiber_switch protects from calling Fiber#resume via mrb_funcall: https://github.com/mruby/mruby/blob/master/mrbgems/mruby-fiber/src/fiber.c#L166

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Dec 24, 2015

Member

mrb_fiber_resume should work from C functions, but it currently does not. I consider it's a bug, but it requires huge API change. Please use proc workaround for a while.

Member

matz commented Dec 24, 2015

mrb_fiber_resume should work from C functions, but it currently does not. I consider it's a bug, but it requires huge API change. Please use proc workaround for a while.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 25, 2015

Contributor

@matz Thank you for the answer. Will use the proc workaround.

@Asmod4n Thank you for the suggestion. Unfortunately as stated by @matz calling mrb_fiber_resume directly does not solve the issue.

fiber_switch protects from calling Fiber#resume via mrb_funcall:

FWIW, to be precise, the code does not protect Fiber#resume from being called through mrb_funcall; the guard exists to prevent fiber switches that involve a C stack. For example if the call stack is C -> mruby -> C* -> ... Fiber#resume, it is impossible to swap the stack of mruby since the second call stack of C (marked with *) cannot be swapped.

A fiber switch should not be deterred unless when a C function is called from mruby; fiber_switch is implement correctly in that respect.

Contributor

kazuho commented Dec 25, 2015

@matz Thank you for the answer. Will use the proc workaround.

@Asmod4n Thank you for the suggestion. Unfortunately as stated by @matz calling mrb_fiber_resume directly does not solve the issue.

fiber_switch protects from calling Fiber#resume via mrb_funcall:

FWIW, to be precise, the code does not protect Fiber#resume from being called through mrb_funcall; the guard exists to prevent fiber switches that involve a C stack. For example if the call stack is C -> mruby -> C* -> ... Fiber#resume, it is impossible to swap the stack of mruby since the second call stack of C (marked with *) cannot be swapped.

A fiber switch should not be deterred unless when a C function is called from mruby; fiber_switch is implement correctly in that respect.

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