[Enhancement] Migrate to coroutines in the interface

[0x00002a]

Taken from my comment here: #70. For tracking and discussion

Moving everything to coroutines might be quite the job. Mostly thinking of the problems mentioned in #40. Also we might have to roll a few of our own coroutines as asio is rather lacking there (awaitable seems to be it, so no tasks or generators).

I'm personally leaning more toward having coroutines (in the interface) in the release after 0.1 as a major API change since I think theres a few parts which could be improved with coroutines (e.g. client::controller::ws_connect could return a connection again, and server handlers could optionally be coroutines).

Parts of the API that might benefit from coroutines, off the top of my head

• client::controller::ws_connect: Could return an awaitable to the connection object
• server router callbacks: Could optionally be coroutines, allowing co_await in the bodies of them
• send, accept, read, etc in websocket::connection: These currently use callbacks, making them coroutines could not only clean things up a little but also make [Bug] websocket::connection::read does not interleave with websocket::connection::send #70 simpler
• client::controller::http_connect currently uses a callback with a response. Since we have the Filter type we can use a templated coroutine return. This does however sacrifice the implicit visitorness of the callback, so that one needs a bit more thought
• Basically anywhere we currently return future

[Tectu]

As discussed previously: I am all pro using coroutines - but unfortunately not unless MinGW-w64 & GCC shipped by MSYS2 can compile malloy.

[Tectu]

I wanted to add this mental note ever since I read the changelog of boost v1.77.0: There are changes to coroutines. Might be worth exploring.
Personally I would be okay with moving boost minimum version to 1.77.0 if we can benefit greatly from that.

[0x00002a]

clang 13 is now out (and in the arch repo) so I may start looking at migrating to corountines. Problem is this is going to break all existing code, and in ways that are not just a search and replace fix either (existing code must also use coroutines or wrap stuff up in asio spawns).

Ideally we'd instead migrate to the ExecutionToken CompletionToken api of asio, problem is it (like the rest of asio) has basically no documentation. This api is what allows passing different execution types to the asio async functions and getting different ways of async notification (e.g. passing a function to async execute or asio::use_awaitable and getting a coroutine back). I'm also not sure if we'd need to reorganize a lot of the internal architecture for this to work, as I don't know how CompletionToken's propagate (or even if they can), which may cause issues with stuff like the queued async actions (at a guess we'd need to type erase the token type at the very least)

[Tectu]

Awesome to hear from you again! :)

I really don't know what to tell you... this is a difficult situation...
Overall I do think that going for proper coroutines is the more future proof way of doing things as this is inevitably going to be supported by the language itself (I mean technically it already is).
Then again, I do agree that forcing the use of coroutines on the user/consuming-application is a bold move. But then again, malloy itself is a C++20 library so it's highly unlikely that you're gonna use this library in a context where coroutines are not available. This just boils down to compiler support.

A couple of months ago I did investigate the boost ASIO stuff but I trailed off mainly due to lack of documentation and then side-tracked into an endless pit of interesting stuff - as one does when looking at boost code.

Ideally we'd instead migrate to the ExecutionToken CompletionToken api of asio, [...]

Could you elaborate as to why you'd think this is the ideal choice?

[0x00002a]

Ideally we'd instead migrate to the ExecutionToken CompletionToken api of asio, [...]

Could you elaborate as to why you'd think this is the ideal choice?

It would allow the user to pick what kind of async interface they wanted (so they could even write their own or use their existing custom one they wrote for asio with this). The immediate advantage in terms of coroutine migration is it would allow malloy to support the use of coroutines without requiring that consumers use them and without breaking existing code (hopefully).

I agree though that it might be better to just support coroutines, all the guides and such on implementing the api require a lot of boilerplate e.g. (a new struct with a bunch of using's for every composed operation, of which there are 2-3 alone for websocket::send for example) and a whole lot of macros. I think even if I could implement it I doubt it would be very readable (although I may be wrong, like I said I don't know a lot about this yet).

Another thing to consider is I thiiink the CompletionToken api is part of the networking ts.

Personally I think I'm going to at least try converting one or two paths of the websocket interface, and see how messy/possible it is (unless you have any major objections of course)

[Tectu]

Personally I think I'm going to at least try converting one or two paths of the websocket interface, and see how messy/possible it is (unless you have any major objections of course)

Of course I don't! I am very thankful for this. As mentioned I did look into it myself and came to similar conclusion as you stated. I think even if we can pull it of (which I don't doubt) it's gonna be a b*tch to maintain.

Another upside of just using straight coroutines would be that beast provides examples on those so we'd have some reference in case we are getting stuck / misunderstand something.

Then there is that other benefit as you mentioned:

The immediate advantage in terms of coroutine migration is it would allow malloy to support the use of coroutines without requiring that consumers use them and without breaking existing code (hopefully).

[0x00002a]

Alright, so I've managed to get ws_connect working with asio::use_future 🎉. The code is in the feat-comp-tkns branch. There are a few issues currently:

• The code is interesting to say the least. However I do think I can wrap everything up into something resembling a readable api (see then and then_builder)
• What I don't think can be avoided is the loss of clear dataflow, asio::async_completion has to be used and its not clear from the code how it works
• Currently can't get awaitable to work, it isn't cooperating with async_completion, see this (github issue)
• The then_builder api needs work, async::start particularly needs renaming (along with possibly the namespace)

Edit:
Currently looking at async_initiate which may fix this + make things nicer to read

[0x00002a]

Okay so I have it working with asio::use_awaitable. Turns out all of the then stuff is not actually needed and sent me on a wild goose chase. It is actually a hell of a lot simpler than I thought: Call asio::async_initiate with:

1. a function that takes the final completion handler as the first arg
2. the final completion handler (e.g. the callback, asio::use_awaitable, etc)

Then just invoke the final handler when done.

Having said that, it does seem to have made my compile times go up to minutes for websockets.cpp, which isn't great. Also clang 13 doesn't seem to work with libstdc++ and coroutines (I think I ran into this before actually), I'm guessing it works with libc++ but I haven't tested it yet.

My vote is that we switch to this at least for all the client stuff, my only concern is the compile times for just the tests have already skyrocketed (at least for me), I think this is actually the coroutines and not the templates (clang, for which the coroutine tests are ifdef'd out, compiles at normal speed). Thoughts?

[Tectu]

Nice work!

I can have a much closer look at this on the 2nd of January. Unfortunately I could only glance over your work so far.
Compile times are a major concern in any case. Yesterday I started looking into using pre-compiled headers (PCH). CMake would support this too but this is also something I have to test after the 2nd of January.

I'm really not sure whether it's worth migrating to the "new" boost::asio stuff given that coroutines will become more widely supported eventually anyway. But I guess this just brings us back to the original conversation.

After your experiments, do you think it's worth pursuing this (leaving compile-times out of the discussion for now)?

[0x00002a]

After your experiments, do you think it's worth pursuing this (leaving compile-times out of the discussion for now)?

Yes. It will allow supporting all current environments + using the cool new stuff as and when it becomes available with no extra code changes from us. I've now migrated all of websocket::connection to use this and client::controller::ws(s)_connect, and it was (mostly) trivial.

Adding support for this only needs a few lines of code in most cases. I did run into a slight issue with the action queues in send/read, turns out std::function copies lambda's when constructed, which is an issue since not all completion tokens are copyable (but are moveable). I had to add some extra code for that, basically manually declared the struct version and passing that instead of the lambda which is a little ugly but not too bad.

Compile times are a major concern in any case [..]

Compile times seem to be a problem only when actually using the coroutines, I'm not sure if its because of complex TMP needed to make them work or gcc's coroutine implementation being a bit slow right now or a bit of both, but the bottom line is if you don't use it you don't pay for it (as far as I can tell). Although I agree that generally malloy compile times are quite high

I've implemented this now for send, read, accept, and connect for websocket connection. Also I've managed to make gcc crash when trying to compile the coroutine tests, which I consider something of an achievement.

I'm really not sure whether it's worth migrating to the "new" boost::asio stuff given that coroutines will become more widely supported eventually anyway. But I guess this just brings us back to the original conversation.

True, but there are cases for this beyond just allowing easier migration to coroutines. The CompletionToken api allows very deep customization of how the control flow is returned to the caller, including error handling which is something I've been eyeing for #40 for some time.

I think the only real sticking point is still client::controller::http_connect, because of the multiple types return. I think we have a few options:

1. Special case the 1-item variant (single body type) to remove the variant, meaning for the majority of cases you could just do auto v = co_await ... rather than then having to call std::visit(..., v). The drawback of this approach is that if you have multiple bodies suddenly a variant appears, also unless we want to special case callbacks, this would break existing code that relies on the callback being implicitly a visitor. I like this option because it provides the expected, simple interface for the normal use case with the more complex case only appearing when the user is doing some complex customisation anyway
2. Always pass/return a variant. Advantages are the api stays consistent when you adapt the filter, disadvantages are it breaks all existing code and makes for always needing to use std::visit even with only one body type. I really don't like this option
3. Keep the implicit visitorness of the callback, but make every other completion token use variant. I don't really like this option as it makes for code like auto v = co_await ...; std::visit(..., v). Also we need to special case callables which I feel breaks the encapsulation of a CompletionToken a bit, and could lead to weridness.
4. Combination of 3 and 1, special case the callbacks but also special case the 1 element variant for other completion tokens.

I'm on the fence between 1 and 4 honestly. But I'm leaning more toward 1 as it is kinda surprising behavior if you change away from your lambda and suddenly need visit

[Tectu]

Yes. It will allow supporting all current environments + using the cool new stuff as and when it becomes available with no extra code changes from us.

I see this not as a major - but reasonably important plus.

True, but there are cases for this beyond just allowing easier migration to coroutines. The CompletionToken api allows very deep customization of how the control flow is returned to the caller, including error handling which is something I've been eyeing for #40 for some time.

I didn't consider that - you're absolutely right. #40 is quite a biggie on my side as well (and probably for anybody who wants to use malloy for anything reasonable beyond PoC-type stuff).

I think the only real sticking point is still client::controller::http_connect, because of the multiple types return. I think we have a few options: [...]

I see what you mean... I truly do...
As per my current knowledge & state-of-mind I would argue that option 1 is the most sensible.

I appreciate your efforts on this front. I think this will be a good step forward and certainly future-proofing.
Personally I really don't mind breaking changes. We're still releasing null-major versions. Things are still experimental. I'd rather have more breaking changes now than later down the road when this library becomes more popular / more widely used.

On that note: We'll most likely get a v0.3 release in the next few days. Maybe the new fancy stuff makes it into v0.4?