[Kde-pim] Async feedback

[Christian Mollekopf]

On Thursday 18 December 2014 15.10:56 Daniel Vrátil wrote:
> On Thursday, December 18, 2014 12:03:01 PM Christian Mollekopf wrote:
> > Hey Daniel,
> > 
> > I meanwhile took a look at the async code, and here's the feedback I have
> > so far.
> 
> Awesome, thanks!
> 
> > First off though, nice job! I think it goes very much in the right
> > direction =)
> > 
> > So:
> > * I think it would be nice to remove waitForFinished from FutureGeneric,
> > offering only a callback. It keeps the baseclass simple and clean, and
> > it's
> > easy to implement a little wrapper that provides blocking waiting if
> > necessary.
> 
> We have FutureWatcher for the callback, and having a "BlockingFutureWatcher"
> does not make much sense IMO.
> 
> I don't think it really matters whether the baseclass is simple or not: the
> only reason we have FutureBase is that I need to store a non-templated
> pointer to Future in ExecutorBase (which is non-templated itself),
> otherwise it would all be in Future.
> 

If it's code we anyways require perhaps, otherwise I disagree.

> > * Currently you're copying all result values to pass it on to the next
> > step, which is something I hoped to avoid. I think setFinished should
> > take the value and directly call the continuation (should be possible
> > from what I've seen).
> 
> We can optimize this using the move semantics etc. I was just being lazy :-)
> > * The final value should be consumable by a continuation:
> > 
> > job.exec().then<QString>(
> > 
> > 	//final continuation consuming the value
> > 
> > )
> 
> +1, I like this. Not sure how to do it yet, but I'll figure out something :)

I actually ended up using it differently.

To write composable code you must always return a job, and never a future, 
because a job could finish immediately (I think it should be allowed to).

If you return a job you can always do:

getJob().then<QString>(
  //final continuation
).exec();

This should IMO be the only default way to consume the value.

In that case the "future" returned by exec is only required for two things:
* as handle to control the job (stop/abort)
* lifetime management (something that seems to be missing so far)

The problem I see with Future, and especially it's isFinished method, is that 
we're advocating an incoherent programming style, where some people may choose 
to return futures from functions, and some may choose to return jobs.

IMO returning jobs is superior in every way:
* jobs are composable, futures are not (you can use the value of course, but 
that's no better than KJob)
* jobs leave it up to the caller to execute it, and thus also leave the 
decision on *how* and *when* to execute to the caller.
* With futures you always have two codepaths (and with jobs always one):
** either the future isFinished and you need to access the result directly
** or you wait for the finished signal
* By providing an accessor we're forced to keep the result value in-memory for 
the lifetime of the future, IMO for no good reason.

The Job class itself I find excellent, especially that we can create it on the 
stack and generally treat jobs like any other value. When it comes to 
execution I would suggest using little wrappers, that define how the job is 
executed and potentially extract the result. This allows for greater 
flexibility, without coupling the Job class to it (so you can just write a new 
wrapper if you need one easily.

i.e. a wrapper that executes the job synchronously:

auto result = extractResult<QString>(getJob());

* implemented in a simple function that directly runs an eventloop to block, 
and in the end returns the result.
* error handling could i.e. be implemented using exceptions, or an additional 
error out argument (the point is, the job implementation doesn't care)

or a more KJob like wrapper that uses signals and is heap allocated for fire 
and forget usage:

auto asyncRunner = new AsyncRunner<QString>(getJob());
connect(&asyncRunner, &resultRunner(QString), ....

If we provide sane default implementations for those wrappers, it's just as 
easy to use, but keeps the coupling lower.

These wrappers can then also be used for proper lifetime management.

Job::exec() should IMO only return a handle to control the job and to manage 
it's lifetime (the job dies with the handle). How exactly the jobs are then 
used API wise is up to the used wrapper. I realize that Future does much of 
that already, but I think it would be a good design decision to explicitly 
separate the result value consumption/extraction from that handle.


> > The continuation would in this case also replace the
> > FutureBase::isFinished
> > acessor I think.
> 
> Probably, but there's no harm in keeping it around. We need the state
> internally anyway.
> 

See above, I think we could get rid of that state.

> > If you wanted a future that stores the value you still could:
> > MemoryFuture<QString> finalFuture(future2);
> > 
> > The memory future would internally copy the value from the continuation,
> > perhaps emit a signal when done, and provide isFinished().
> > 
> > * We may want to add support for more convenient composition:
> > 
> > getJob1().then(getJob2())
> > 
> > Currently you have to do something like:
> > 
> > getJob1().then([this](const QString &arg, Async::Future<void> &future) {
> > 
> >   getJob2().exec().then<void>([future]() {
> >   
> > 		future.setDone();
> > 	
> > 	}
> > 
> > });
> > 
> > Which is fine for now and works nicely with kjobs, but perhaps it would be
> > worth it to improve on that sometime? Of course the convenient composition
> > as above would only work if the result type of the first job and the
> > argument type of the second job are a perfect match, so perhaps it's also
> > not worth it.
> 
> Indeed. I used to have overloads for then(), each() etc. that accepted Job
> as an argument, but they got lost over the time. I'll see if they can be
> brought back.
> 

Cool, it's more of a nice to have anyways.

> > * Error handling is so far missing. Each step should be able to provide an
> > error handler, and by default errors should bubble up, so you only have to
> > handle errors at the end (probably as an error handler continuation of the
> > future).
> 
> Yep, I hoped to do it this week, but so far spent most time visiting
> relatives
> :D Will work on that.
> :

Will you have time for that anytime soon? Otherwise I might look into it.

> > * We probably should have a way to abort a job (if the job supports that
> > of
> > course).
> 
> ditto
> 
> > * Execution is so far tied to the event-loop, and I'm wondering whether we
> > should also support non-eventloop execution, i.e. to be executed in a
> > thread. Can we specialized the executors for that?
> 
> Indeed that should be rather easy. We still need the eventloop though, you
> probably don't want a blocking wait for Future to finish.
> 

... which is another reason why I was wondering whether setFinished can 
directly call the continuation =)

Or we register a continuation for when the future is ready.

> > Otherwise it looks really nice =) Let me know what you think about the
> > points above and whether you plan on working on them, otherwise I might
> > give it a try as well ;-)
> 
> It's a small codebase and since it's all template magic, even small changes
> tend to break everything :D I will first do the error handling and aborting,
> then we can move on with the remaining stuff.
> 
> Dan
> 
> > Cheers,
> > Christian
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/