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

[jClaireCodesStuff]

coroutine_unwind was doing undefined things.

I've replaced it with an AtomicBool flag triggering the same unwind code moved to yield_with. This isn't optimal yet (I'll try to get it passing through the data field as zonyitoo suggests) but it does pass all tests even with #[inline(always)]

Still needs:

• data field
• decide on proper inline attribute
• out-line the cold branch (probably)
• delete commented-out code

[lhecker]

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.

[zonyitoo]

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.

[lhecker]

I think we might have to check for the unwind flag here too and in that case directly proceed to here. Is that correct, @zonyitoo?

[zonyitoo]

Theoretically, we don't need to check it here, because it is impossible to trigger any unwinding process before callback(), all codes are in spawn_opts.

[jClaireCodesStuff]

I tried simply checking for data != 0 to trigger unwinding.

It breaks somewhere and I'd like to have the logger working to figure where.

[lhecker]

@glasswings it does not work because you'd need to check for data == X where X must be some invalid address except for 0.
The reason for this is that every Coroutine implementation needs a concept of "taking a coroutine out" of the run loop. In our case this happens using the Process::park_with() method, which uses the data field to pass a pointer. You thus need use a value which would be invalid as a pointer (like usize::max_value()) and check for exactly that value.

[jClaireCodesStuff]

I see. I misunderstood the topology. It's not one Processor context which activates each Coroutine in turn; there's a ring of running Coroutines. The data field is overloaded.

It (may?) also mean the Handle::drop event can happen in any of those contexts.

---

On master:

---dropping context--- 
Handle::drop sees !State::Finished 
and continues on top to
---dropped context--- 1
coroutine_unwind unwinds through
...all callback code to...
coroutine_entry sets State::Finished 
and continues to
---dropping context--- 2
Handle::drop sets State::Dropping
and continues to 
---dropped context---  3
`coroutine_entry checks for State::Dropping 
and continues on top to
---dropping context--- 4
`coroutine_exit` which manually drops Coroutine
which decreases a counter 
then it returns the stack to the pool
and returns to
Handle::drop which 
returns

The function of data passed to coroutine_unwind is to ensure that context-switch 2 happens. coroutine_unwind needs to follow this pointer to get to the Coroutine struct living at the bottom of its stack.

---

I think you're saying, and I'll next try:

---dropping context--- 
Handle::drop sees !State::Finished 
and passes usize::MAX continues to
---dropped context--- 1
yield_with, which sees usize::MAX and unwinds through
...all callback code to...
coroutine_entry sets State::Finished 
and continues to
---dropping context--- 2
Handle::drop sets State::Dropping
and continues to 
---dropped context---  3
`coroutine_entry checks for State::Dropping 
and continues on top to
---dropping context--- 4
`coroutine_exit` which manually drops Coroutine
which decreases a counter 
then it returns the stack to the pool
and returns to
Handle::drop which 
returns

yield_with has a self pointer and doesn't need the pointer through data.

---

Maybe we can save the middle two context switches

---dropping context--- 
Handle::drop sees !State::Finished 
sets State::Dropping
and passes data = MAX
and continues to
---dropped context--- 1
yield_with, which sees data = MAX and (debug State::Dropping) and unwinds through
...all callback code to...
coroutine_entry checks state and keeps Dropping (or sets State::Finished)
and continues on top to
---dropping context--- 4
`coroutine_exit` which manually drops Coroutine
which decreases a counter 
then it returns the stack to the pool
and returns to
Handle::drop which 
returns

yield_with shouldn't wait on a cache miss reading self.state when it's resuming normally.

[lhecker]

@glasswings How's it going? Should one of us take a look? We could continue development of this as well if something is left. And if you want to: My offer of introducing you how all of this works is still there. 😄

[jClaireCodesStuff]

I'm feeling pretty happy with the code as it is. It would be nice to pass the network echo tests, too, but I don't think I broke them. The added comments should definitely be reviewed to be sure they are not misleading. They are a little too verbose and I wouldn't be offended if they're edited down.

All-in-all it's ready for a look.

I pushed the faster coroutine teardown (two fewer context switches) to my fork. It seems to work just as well at first glance, but I'm not likely to mess around with it much further.

My roommate has expressed renewed interest in learning programming, and I'll be shifting my time to things we can work on together.

So I'm planning to see this issue through to resolution and, if it's not too frustrating, troubleshoot why the network echo tests aren't working on MSYS/Win10. Then moving on to other things. Thank you both very much for making me feel welcome.