Tectu
commented
2 years ago Wow... that is a substantial amount of work... I'll have a proper look at this today/tomorrow/... (should be fine as I should have a lot more time at hand the next few weeks).
One thing I noticed while just browsing this: It would seem that the Ubuntu test-suites on boost versions >= 1.74.0 hang (from the quick looks of it if it's either a shared build or a TLS-enabeld build (?)). This would be similar to #89
This implements support for the asio CompletionToken api as discussed in #74.
There are some issues currently with coroutines. The only compiler I can get working with them currently is gcc and it won't compile one of the tests (it crashes with an ICE). There is an example using coroutines, but it throws a
bad_allocexception with an unreadable stack trace, I'm not sure if this is an issue with how I wrote the example or asio.Major breaking changes
client::controller::http_connectno longer accepts a visitor, instead if the filter has a single body type it accepts a completion token for a response with that body, otherwise a completion token with a variant of responses over the possible bodies. This means that code currently using callbacks and filters with more than 1 body (i.e. a custom one, none of the ones packaged with malloy currently use this) need to add a call tostd::visitclient::controller::http_connectno longer returns astd::future<error_code>, instead it is contained in the completion token. This means every user currently using a callback token (all of them) needs to add another argument. The reason it originally returned a future was to prevent the user having to write the same boilerplate every time to wait on the result without running off the end ofmainand without waiting forever on an error. However, the future behavior can be restored by passingasio::use_futureand then adding atry...catcharound the.get(), which is what I have done for the examplesOther major changes
websocket::connection::[accept, send, read, connect]now use CompletionToken for the template type, allowing one to pass for exampleasio::use_futureand have the return be a future rather than invoking a callbackNotable internal changes
action_queuenow uses a customactionpolymorphic interface for the stored actions. This is in order to allow non-copyableCompletionTokens as converting tostd::functioncauses a copy (as I discovered). While this does require the explicit allocation of aunique_ptrI believe it actually results in fewer allocations overall asstd::functionhas a lot of hidden ones. This does meanwebsocket::connectionhas some additional boilerplate but I consider this a reasonable tradeoff for the api it now providesasio::async_initiateis now used to enable the CompletionToken api. One gotcha with it is that the type passed to the handler function is not the same as the CompletionToken type, and is not guaranteed to by copyable (although it is always moveable)client::http::connectionhas changed a bit,runhas been removed with its arguments being moved to the constructor.startis now called to start it. I originally tried to remove it completely but due toshared_from_thisit can only be started after the call tomake_sharedis complete. Honestly I am not particularly happy withconnectionin its current form, as it isn't really a standalone class and is essentially just a complex delegate that should be an implementation detail ofcontrollerimo (unlikewebsocket::connectionfor example, which is completely usable standalone).Things that could use improvement (IMO)
controller::stopalso use this api? It might be doable but is more complex because of the weird inheritance stuff going on with ituse_future, there is one currently for websockets but I'm not sure if there is any advantage to more (and the disadvantage is effectively duplicate code with only minor differences)