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

Issue 51 Unwind error if the Coroutine::yield_with is inlined #52

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions src/coroutine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ extern "C" fn coroutine_entry(t: Transfer) -> ! {
next: None,

stack: Some(stack),

unwind_flag: ::std::sync::atomic::AtomicBool::new(false),
};

{
Expand Down Expand Up @@ -98,7 +100,10 @@ extern "C" fn coroutine_exit(mut t: Transfer) -> Transfer {
t
}

extern "C" fn coroutine_unwind(t: Transfer) -> Transfer {
//extern "C" fn coroutine_unwind(t: Transfer) -> Transfer {
// Inlining actually didn't trigger issue 51 on MINGW64, so
// stubbing this out.
/*
let coro = unsafe { &mut *(t.data as *mut Coroutine) };

// Save the Context in the Coroutine object because `coroutine_entry()` needs
Expand All @@ -107,7 +112,9 @@ extern "C" fn coroutine_unwind(t: Transfer) -> Transfer {

trace!("{:?}: unwinding", coro);
panic::resume_unwind(Box::new(ForceUnwind));
}
*/
// t
//}

#[derive(Debug)]
pub struct ForceUnwind;
Expand Down Expand Up @@ -136,6 +143,10 @@ pub struct Coroutine {
next: Option<Handle>,

stack: Option<Stack>,

// Proof-of-concept
unwind_flag: ::std::sync::atomic::AtomicBool,

}

unsafe impl Send for Coroutine {}
Expand Down Expand Up @@ -228,15 +239,33 @@ impl Coroutine {
//
// Tracking issue: https://github.com/zonyitoo/coio-rs/issues/51
#[doc(hidden)]
#[inline(never)]
#[inline(always)]
pub fn yield_with(&mut self, state: State, data: usize) -> usize {
let context = self.take_context();

trace!("{:?}: yielding to {:?}", self, &context);

self.state = state;

// yield ...
let Transfer { context, data } = context.resume(data);
// ...continue

// Check if it is time to unwind
if self.unwind_flag.load(::std::sync::atomic::Ordering::Acquire) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you read my other comment about passing usize::max_value():
Here you could now check for data == usize::max_value() (generated by the drop() method) and in that case unwind the stack.
The data value is currently only used to pass a function pointer to the Processor for the Processor::park_with() method (which might get replaced with resume_ontop() in the future anyways).
It will thus never yield a invalid address like 0 or usize::max_value() (those are errors values in POSIX anyways).

BTW: You don't need to synchronize internal state using atomics. Coroutines are inherently not thread safe and the current thread owns them to it's full extend. Whenever Coroutines are transferred from one thread to another the memory access will be synchronized using atomics/barriers anyways already.

// contents from `coroutine_unwind`

let coro = unsafe { &mut *(data as *mut Coroutine) };

// Save the Context in the Coroutine object because
// `coroutine_entry()` needs to get a hold of the callee Context
// after exiting the callback.
coro.context = Some(context);

trace!("{:?}: unwinding", coro);
panic::resume_unwind(Box::new(ForceUnwind));
}


// We've returned from a yield to the Processor, because it resume()d us!
// `context` is the Context of the Processor which we store so we can yield back to it.
Expand Down Expand Up @@ -315,7 +344,9 @@ impl Drop for Handle {
if state == State::Finished {
ctx.resume(0);
} else {
ctx.resume_ontop(self.0 as *mut _ as usize, coroutine_unwind);
self.unwind_flag.store(true, ::std::sync::atomic::Ordering::Release);
//ctx.resume_ontop(self.0 as *mut _ as usize, coroutine_unwind);
ctx.resume(self.0 as *mut _ as usize);
Copy link
Collaborator

@lhecker lhecker Apr 30, 2016

Choose a reason for hiding this comment

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

This is the point where previously we passed the self pointer as the data field to the coroutine_unwind method. Now you could for instance resume the execution of the coroutine with a custom value like usize::max_value() right here. The Coroutine will resume where it left off and that's the yield_with() method.

Copy link
Owner

@zonyitoo zonyitoo May 1, 2016

Choose a reason for hiding this comment

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

Yes, you can pass a specific value to the data field to indicate that you are going to do force unwinding. The data will be returned from the resume in yield_with.

Also, you can use the state field, for instance, you can add a State::ForceUnwind and set it to state before this ctx.resume right here, and check the self.state in yield_with.

BTW: There is no need to use atomic flag here, because it is impossible to access one Coroutine from multiple threads, which is ensured by the other parts of coio.

}
}
}
Expand Down