Re: [boost] [afio] Formal review of Boost.AFIO

Subject: Re: [boost] [afio] Formal review of Boost.AFIO
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2015-08-29 16:30:16

On 08/29/2015 02:05 AM, Niall Douglas wrote:
> On 28 Aug 2015 at 10:16, Thomas Heller wrote:
>
>> I took the effort to also reimplement your hello world example:
>> https://gist.github.com/sithhell/f521ea0d818d168d6b35
>
> Firstly thank you very much for your gist. This makes it much easier
> to understand what you are talking about.
>
> My very first thought was that you are using reference capturing
> lambdas in a concurrent use case which may be fine in internal
> implementation, but is a serious no-no to ever place upon a library's
> end users. People regularly get waits and wakeups wrong, and I have
> seen code bases wrecked by memory corruption and race conditions
> induced by reference capturing lambdas used in concurrent situations.

All true. This contrived example is not really concurrent though, but 
whatever. Please, stop treating your users as they wouldn't know what 
they do.

>
> For me this is an absolute red line which cannot be crossed. I will
> never, *ever* agree to a library API design which makes end users
> even feel tempted to use reference capturing semantics in a
> concurrency context. Period. They are banned as they are nuclear
> bombs in the hands of the typical C++ programmer.

Sure, whatever, this is just derailing ... You do realize though that 
you demote your users library to idiots not knowing what they do? 
Despite promoting your library as something very niche only used by experts?
This is by giving up your own promoted "Don't pay for what you don't 
use" principle. But let's move on...

>
> So let's replace all your reference captures with shared_ptr or value
> captures and thereby making them safe. I have forked your gist with
> these changes to:
>
> https://gist.github.com/ned14/3581d10eacb6a6dd34bf
>
> As I mentioned earlier, any of the AFIO async_* free functions just
> expand into a future.then(detail::async_*) continuation which is
> exactly what you've written. So let me collapse those for you:
>
> https://gist.github.com/ned14/392461b85e73add13e30

Where does error handling happen here? How would the user react to 
errors? What does depends do (not really clear from the documentation)? 
How do I handle more than one "precondition"? How do I handle 
"preconditions" which have nothing to do with file I/O?

Which ultimately leads me to the question: Why not just let the user 
standard wait composure mechanisms and let the functions have arguments 
that they really operate on?
All questions would have a simple answer then.

NB: I am still not sure what afio::future<>::then really implies, does 
it invalidate the future now? When will the continuation be executed, 
when the handler future is ready or when the "value" future is ready? 
What does that mean for my precondition?

>
> This is almost identical to my second preference API for AFIO, and
> the one Gavin Lambert suggested in another thread.
>
> As I mentioned in that thread with Gavin, I have no problem at all
> with this API design apart from the fact you need to type a lot of
> depends() and I suspect it will be a source of unenforced programmer
> error. I feel the AFIO API as submitted has a nice default behaviour
> which makes it easy to follow the logic being implemented. Any
> depends() which is a formal announcement that we are departing from
> the default stand out like a sore thumb which draws the eye to giving
> it special attention as there is a fork in the default logic.

That's exactly the problem: You have a default logic for something that 
isn't really necessary, IMHO. The async operation isn't an operation on 
the future, but on the handle. And to repeat myself: That's my problem 
with that design decision.

>
> Under your API instead, sure you get unique futures and shared
> futures and that's all very nice and everything. But what does the
> end user gain from it?

1. Clear expression of intent
2. Usage of standard utilities for wait composures
3. Transparent error handling
4. "Don't pay for something you don't use"

> They see a lot more verbiage on the screen. They find it harder to
 > follow the flow of the logic of the operations and therefore spot
 > bugs or maintain the logic.

Verbosity isn't always a bad thing. Instead of trying to guess what is 
the best default for your users stop treating them as they would not 
know what they do.

> I am seeing lots of losses and few gains here apart from meeting some supposed design
> principle about single responsibility which in my opinion is a useful
> rule of thumb for inexperienced programmers, but past that isn't
> particularly important.

You do realize that this is highly contradicting to anything else you 
just said in your mail? You claim to have your library designed for the 
inexperienced user not knowing what they do, yet your design choice 
violates principles that are a nice "rule of thumb for inexperienced 
programmers"?
Beside that, single responsibility should *always* be pursued. It makes 
your code testable, composable and easier to reason about.

>
> I however am probably being a little unfair here. I reduced your
> original to something AFIO like and something I previously gave deep
> consideration to adopting before rejecting it, and you were probably
> actually advocating that people manually write out all the .then()
> logic instead of using free functions which expand into exactly the
> same thing.

Right, dependency handling is something inherhent to the users code. 
Anything you'll come up with inside of your library will be the wrong 
thing in some cases and then you'll have a problem.

>
> If you really strongly feel that writing out the logic using .then()
> is very important, I can add that no problem. Right now the
> continuation thunk types live inside a detail namespace, but those
> can be moved to public with very little work.
>
> Niall