dpc / mioco.pre-0.9

Scalable, coroutine-based, asynchronous IO handling library for Rust programming language. (aka MIO COroutines).
Mozilla Public License 2.0
457 stars 30 forks source link

sync_channel #148

Closed tekjar closed 8 years ago

tekjar commented 8 years ago

Hi. I don't know much about the architecture of mioco but I tried to replicate Sender code to add SyncSender. I'm not sure about the implications but example seems to be working on linux.

dpc commented 8 years ago

This implementation will work only if sender is used outside of mioco. If SyncSender is used inside of mioco it must mever block on send().

The most naive (but acceptable for now) solution would be to use:

https://doc.rust-lang.org/std/sync/mpsc/struct.SyncSender.html#method.try_send

if running inside mioco (can be detected with http://dpc.pw/mioco/mioco/fn.in_coroutine.html), use try_send and if channel was full, just http://dpc.pw/mioco/mioco/fn.yield_now.html to retry later. It's not optimal but it will work well enough and technically correct.

Also, the current policy is that there must be some unit-test for every feature. In this case:

After that I'd be happy to merge. Thanks for your contribution.

tekjar commented 8 years ago

Got it. Thanks for the review. I'll make the changes you suggested :)

dpc commented 8 years ago

Nice. Tests and everything. We'll help with investigating the inside-inside issue, and then we can merge.

dpc commented 8 years ago

Using writeln!(&mut std::io::stderr(), "{}. #### Sending ...", i); in tests instead of println! is much better way, as println is buffered.

I think I've found the issue, BTW.

dpc commented 8 years ago

I've posted comment. After fixing that, all tests pass. Sorry for going back to you after a longer time. I'm just extremely occupied with things.

Anyway, please fix, cleanup, check again, and I'll be super happy to merge! Looking forward to merge more.

dpc commented 8 years ago

I added a cleanup patch and pushed to master. Thanks!

tekjar commented 8 years ago

Sorry. I didn't get time to look into this. Thanks for taking it up :)