jbaldwin
`unique_ptr<io_scheduler>&` interface seems too restrictive
For multiple functions (i.e. tcp/server tcp/client) a std::unique_ptr<coro::io_scheduler>& is asked as scheduler type. This enforces the caller to use unique_pointers. On the other hand, if you pass this to a coroutine, it conflicts with the CPP core guideline that you should not use reference types in coroutines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#cp53-parameters-to-coroutines-should-not-be-passed-by-reference
Would a coro::io_scheduler* raw pointer not be more flexible to the user?
Note
This problem does not apply to reference parameters that are only accessed before the first suspension point. Subsequent changes to the function may add or move suspension points which would reintroduce this class of bug. Some types of coroutines have the suspension point before the first line of code in the coroutine executes, in which case reference parameters are always unsafe. It is safer to always pass by value because the copied parameter will live in the coroutine frame that is safe to access throughout the coroutine.
Thanks for pointing this out, it does indeed seem incorrect and using the pointer is likely the correct method to make sure the pointer value lives on the coroutine stack since std::shared_ptr<> has its own problems and does not work.
I think the std::unique_ptr<>& generally works if you create a scheduler in main and use it for the lifetime of the program since it is a stable reference across coroutines, it is likely a problem when you create and object on a coroutine frame and then reference it on another coroutine. I had mostly envisioned that the schedulers/thread pools would be mostly program lifetime objects, but that is a weird implicit restriction on their use so adjusting this is probably the right call.
Probably the best 1:1 change is to use raw pointers, basically the same as the references now, but you could get nullptr exceptions instead of dangling references, I'd view these as about equally likely so it seems like a good swap to prevent the coroutine dangling ref problem.
The other option would be to go back to std::shared_ptr<> and then use std::weak_ptr for the interface, this would be safer, but incur a large cost if the user is constantly promoting the weak ptrs to shared ptrs. I still think the main use case for these objects are generally long lifetimes on stable stacks, so raw pointers are probably reasonably safe.
Ok I did some more digging and I'm not sure the cpp guidelines are telling the whole truth and seem to blanket just say "don't do this" without explaining why its bad, this stackoverflow post I think outlines the problem better:
https://stackoverflow.com/questions/77996491/c-coroutines-and-const-reference-parameters
It is a problem when a const& or ``&is passed in as a temporary, e.g.make_task(Obj{})and thus the parameter is destroyed at the firstco_awaitsuspension point since the function effectively _returns_. I don't think anyone is or should be passingmake_task(coro::io_scheduler::make_unique())` into a coroutine.. but I suppose it isn't impossible.
Does coro::io_scheduler* prevent this?
auto task = make_task(coro::io_scheduler::make_unique().get());
Ok this is quite dumb, and obvious the unique pointer is going to destruct itself, but its technically possible 😂 . It definitely looks more wrong than a &.
My thoughts here are because I started to do this and it is a massive change and I'm not really sure this is worth it for the executor types (io_scheduler and thread_pool).