Mir

Mir client API lacks simple synchronous interface

Bug #1112195 reported by Daniel van Vugt
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mir
Fix Released
High
Daniel van Vugt

Bug Description

Looking at mir_client_library.h, it seems we've failed a basic rule of API design:
  "Doing simple things should be simple"

Everything is asynchronous. That's useful in uncommon use-cases, but most of the time people just need and want a simple synchronous API:
  conn = mir_connect(...);
but instead you currently have to implement a callback function and pass it to mir_connect, just to get your connection handle.

Callbacks should be optional. They over-complicate things for common use-cases. It should always be simple to do simple things.

Related branches

description: updated
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

To clarify, I suggest a simple split. Functions that are presently asynchronous should have two versions. For example:
  mir_connect()
  mir_async_connect()

Unless someone can think of a way to keep them as one function in an elegant way?

Revision history for this message
Michi Henning (michihenning) wrote :

There is a generic underlying mechanism that can be used. I implemented this for Ice. The API was inspired in part by the .NET API for asynchronous methods. With templates, it's possible to transparently implement the synchronous API in terms of the asynchronous one (with essentially zero overhead).

Basically, the API consists of begin_<method> and end_<method> calls. I start the call by calling begin_<method>() and, when I'm ready to collect the results, I call end_<method>(). If, at the time I make the end_ call, the invocation has completed, the end_ method returns immediately; otherwise, it blocks until the result is available.

This mechanism makes it possible to construct synchronous calls on top of asynchronous calls very easily and elegantly. There are hooks that can be use to type-safely move state between the invoking end and the responding end, and the end_ method can easily be generated by a template to invoke a callback.

My recommendation would be to *not* use callbacks as the default. It is much harder to implement synchronous behavior in terms of callbacks than it is to implement asynchronous behaviour in terms of a completion method. An API along the lines of .NET keeps the flexibility, causes minimum overhead, and doesn't force an awkward interaction model on the application.

http://www.zeroc.com/articles/csharp-async.pdf

Revision history for this message
Michi Henning (michihenning) wrote :

Sorry, the more appropriate link is probably the one below, which shows the C++ API:

http://www.zeroc.com/articles/cpp-async.pdf

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Nice, Michi. An "end" function for each operation would certainly avoid the need for callbacks even in the asynchronous case. And each operation's "end" function can return different data types, unlike the generic mir_wait_for().

I'm guessing it would look like:
    op = mir_begin_something(...);
    ...
    result = mir_end_something(op, ...);

That would be a great way to reduce pain and avoid callbacks in the client API. I still suspect the simple synchronous case requires an extra simple version though:
    result = mir_something(...);

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Of course, one thing you can't do with the "end" approach is wait on multiple things at once. Like you could with a:
    void mir_wait_for_all(MirWaitHandle **wait_handle);

This is one of Windows' strengths, generalizing the Event concept so you can wait on different types of events. Similar for Unix/Linux if everything you're waiting on is a select'able file descriptor.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Sorry, I was wrong. If you use MirWaitHandle as the "op" type above then you could implement wait-for-many functionality.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

There is a simple way to combine all these approaches and do away with callbacks...

    Something result;
    MirWaitHandle *w = mir_something(..., &result);
    ...
    mir_wait_for(w);
    use_result(result);

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Bumped to High. No matter how you look at it, the client API needs more work to be simpler and more digestible.

Changed in mir:
importance: Medium → High
Revision history for this message
Michi Henning (michihenning) wrote :
Download full text (3.5 KiB)

With the begin/end approach, waiting for multiple things is easy. For example, I can fire off five asynch calls and wait for all of them to complete like this:

a = begin_a();
b = begin_b();
c = begin_c();
a2 = begin_a();
b2 = begin_b();

This calls a() and b() twice (just to illustrate that this is possible), and c() once. Now I can wait like this:

a_r = end_a(a);
b_r = end_b(b);
c_r = end_c(c);
a2_r = end_a(a2);
b2_r = end_b(b2);

The last end_ call completes only once all the preceding ones have completed. (I can write the end_ calls in any order; the effect is the same.)

Implementing a synchronous call on top of the async version is trivial. (Illustrated here with an int return type.)

int
someCall()
{
    return end_someCall(begin_someCall());
}

If you want callbacks, you allow a functor to be passed as the first parameter of the begin_ call. The implementation of the overloaded begin_ method stashes the AsyncResultPtr (which is a smart pointer) into a queue or some such, together with the functor. A separate thread (or thread pool) then iterates over the queue and calls the functor, whose implementation calls the end_ method. When the end_ call completes, the functor calls the callback. Once the callback completes, the thread re-joins the thread pool.

In a nutshell, that's all there is to it. In Ice, I wrote much of this by writing the C++ API on the fly from the interface definitions. Internally, it was done mostly with templates, so there wasn't that much code to generate, actually. (Or, rather, I got the compiler to do much of the dirty work for me.)

There are a number of nice aspects about the begin_/end_ approach:

It's stateless, so I can have as many outstanding async calls as are needed without having to jump through hoops.

I can very easily fire off a call, do some more work, and then synchronously wait for call completion when it suits me, *without* having to write a callback. (I can just call the end_ method wherever it's convenient.)

If I want a callback, that's easily arranged for. Moreover, by using functors, there is no need to derive from a common base. (I really dislike designs that force the API client to derive from something I provide.)

I can have one thread start the call and have another thread complete the call, so there is no threading policy imposed on the application. All I need to do is pass the AsyncResult from the calling thread to the completion thread.

It's easy to move application state between the calling end and the completion end.

Everything can easily be made as type-safe as I like, to point where the async call is just as type-safe as a synchronous call.

I don't have to make things type-safe. A single method an be used to process results from an arbitrary number of method invocations, if that's what I want, by providing the method name as a string.

Providing all of this for MIR is probably over-kill. Much of what's in this machinery arose from Ice, which naturally has a type-unsafe dispatch mechanism beneath the covers (the Ice run-time), with a type-safe veneer layered on top by generating C++ code from interface definitions.

For MIR, it probably would be sufficient to stick with a hard...

Read more...

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Yeah I got most of that from the previous comments. Though the client API being a (hopefully) widely used API must be C and not C++.

The client API actually has a common "end" function called "mir_wait_for". Although it lacks type-safe result return. I am leaning more toward the (equally type safe) approach of addressing the result in "begin" (comment #7).

Also, sequences of bespoke "end" functions are potentially a hindrance. Because there might be cases where you want to keep processing as soon as the fastest operation completes. But you don't always know which one that will be. Hence my suggestion of: "mir_wait_for_all" or some-such.

Revision history for this message
Michi Henning (michihenning) wrote :

If the client API is C only, I guess the #7 approach would work. With C++, it's problematic though because it requires the result to be default constructible. It also won't work with polymorphism where the actual type of the return value cannot be known in advance, unless the call returns a pointer. This might be a potential problem if both C and C++ APIs may be needed in future?

wait_for_all() would work, but it seems a little awkward. For one, I'm not sure that the use case is all that common. It would seem unusual for someone to fire off multiple async calls and start processing return values as soon as any one of the calls completes.

Doing so would make sense only for calls that have execution times with large variance, and for calls that return large datasets. But, in those cases, I can use different threads to achieve the same thing or, alternatively, have a callback invoked from the thread pool, so it's possible for several completion callbacks to execute concurrently.

So, I'm not totally convinced that a select() style wait API call would be needed. The few times where I found myself wanting to do something like this, I've used a thread pool, or had different threads make the calls. And select() is sooo eighties… ;-)

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Is this "Bug" any more than an opinion?

I'm not saying that we will never need a sync API, but where does this requirement come from? When the MIR API was first discussed it was clear from a number of sources that the requirement was for an async API.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

The requirement comes from me looking at it. It's pretty obvious we'll need a better API before we can or want to build anything significant with it (hence my job).

Once the client API is public it will be rigidly fixed, so its design is very, very important. More so than the protocol or anything else beneath it.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Ok, ignore wait_for_all and select-style waiting. I was just using it as an argument for how to support diverse concurrent behaviour without needing threads. It is not an immediate requirement. And anything we don't need should certainly not be implemented yet.

Revision history for this message
Michi Henning (michihenning) wrote :

I'd be happy to put my five cents worth in with respect to the API design.

As to the select() style API, my gut feeling is that it's cleaner and easier to do the same thing with threads. The whole select() interface primarily came about because there were no threads back then.

For truly large numbers of things to monitor, threads won't do the job. But select() doesn't do the job either because it doesn't scale. We used epoll() in Ice to get scalability. (We stopped testing after we had more than 100000 connections into a single server process because we ran out of hardware resources to test with, not because Ice wouldn't scale any more.)

But, for async calls, scalability is not an issue. I doubt that people would ever have more than a handful of results outstanding at any one time.

I'd be inclined to hold any select() API at arm's length until there is clear use case for it, and then design an API to meet the use case, rather than designing something to meet an anticipated need that might never come about.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Don't get me wrong, I love threads and even implemented a complete pthread library for Windows in a previous job.

But the problem with threading of any sort is that educated, knowledgeable software engineers usually lack the discipline and experience to use them safely. So if you can provide an API that avoids that danger, fewer bugs will result.

Revision history for this message
Michi Henning (michihenning) wrote :

Yes, I definitely take your point about (mis-)use of threads :-)

In the absence of threads, a select() API would be useful if people want to fire off several async calls *and* wait for all of them at once so they can process results for whichever call completes first. For that to be profitable, I think a few conditions need to be met:

- The time it takes for the individual calls to complete is unknown a priori, or it is highly variable at run time so it's not possible to know in advance which call will complete first.

- The difference between the quickest and the slowest call duration must be large, otherwise the approach isn't worth pursuing. (If they all take about the same amount of time, it doesn't matter which one completes first.)

- The processing of the result data must be expensive (which implies a large result set) or, alternatively, there must be a dependency on the result of a fast call that the application needs before it can proceed, but the result of a slow call won't delay the application unduly, otherwise the approach isn't worth pursuing.

These conditions strike me as unlikely. For starters, I suspect that it'll be rare that an application will have several async calls outstanding to begin with. And, if the application does have several outstanding calls, it seems even less likely that it then will want to process the results of whichever call completes first, instead of waiting for them to complete in some fixed order and process the results in turn.

Also, if an application is built in such a way that it indeed has the kind of dependency I described above, couldn't I argue that the application goes about doing things in a rather strange way?

So, I'm still finding it difficult to come up with a scenario that really requires a select() call of some kind. It seems that the simple begin/end mechanism is sufficient to meet the needs of almost everyone.

If there is case where select() really is essential, I have a feeling it might be better to wait until that case actually arises. Certainly, I wouldn't see select() as a core piece of functionality for async calls.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Absolutely not select(), ever. We can build a much nicer abstraction, if the need ever arose.

However consider when we have an input layer working in Mir. Input events are asynchronous events too - slow and sporadic things that you can't predict. A simple Mir client might accept user input from mouse/keyboard/touch, and all the while have to render itself on screen. So the use case is how to do that all in a single thread. You need an event loop where an event can come from arbitrary sources. And the MirWaitHandle as it exists today is that generic event type.

Revision history for this message
Michi Henning (michihenning) wrote :

Ah, I think I get it now. So, the client essentially runs async invocations along the lines of read_from_keyboard(), wait_for_touch_event(), and so on and then synchronously reacts to whichever happens first instead of having a callback invoked?

I guess that justifies something like wait_for() where I can wait for any one of a number of outstanding calls to complete.

I would probably still prefer a thread pool inside Mir that would be responsible for invoking callbacks but, not knowing anything about the internals of Mir, that may well be inappropriate. (There is also the issue of thread affinity, for example, for GL rendering where the same thread has to make all the rendering calls.) We did solve this particular issue in Ice, where there were similar concerns; the solution was to allow the application to define whether it wanted to service incoming requests (similar to completion callbacks) from a common thread pool, or whether specific kinds of requests were to be served from a separate pool. By setting the pool size to 1, you got single-threaded operation, with the same thread making the callback each time, and without losing concurrency with other incoming requests.

Pool creation and the like was hidden from the application. All the application had to to is create a receiver object and (as a run-time configuration item) set a thread pool size for each receiver object; the Ice run time did the rest.

Anyway, I'll shut up now. I don't know enough about Mir's internal design and requirements to know whether such a design would be appropriate :-)

Changed in mir:
assignee: nobody → Daniel van Vugt (vanvugt)
Revision history for this message
Chris Halse Rogers (raof) wrote :

I agree that the current API is annoyingly complex for the simple cases; having to have a trivial callback for each call is awkward.

That said, I'm pretty sure the complex case needs callbacks, not just the ability to wait on results, and the current merge proposal doesn't provide for them.

Revision history for this message
Chris Halse Rogers (raof) wrote :

I quite like the .NET inspired variant described above - where the asynchronous code is something like

******************
void connect_callback(State cookie, void *ctx)
{
  connection = end_connect(cookie);
}

{
  ...
  begin_connect(stuff, bits, callback, NULL);
  ...
}
******************

and the synchronous code is

{
   ...
  connection = connect(stuff, bits);
  ...
}

where the connect() function is simply

connect(Stuff stuff, Bits bits)
{
  cookie = begin_connect(stuff, bits, NULL, NULL);
  return end_connect(cookie);
}

Revision history for this message
Chris Halse Rogers (raof) wrote :

(Obviously in that example end_connect blocks until the connection has finished)

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Yes my first proposal did implement that begin/end approach. But there are other approaches that keep callback support if it's really required. Coming soon...

Changed in mir:
status: New → Opinion
information type: Proprietary → Public
Changed in mir:
status: Opinion → In Progress
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir at revision None, scheduled for release in mir, milestone 0.0.2

Changed in mir:
status: In Progress → Fix Committed
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Reset ti "in progress" as the linked branch that got committed was an example of how to resolve the issue, not the full resolution.

Changed in mir:
status: Fix Committed → In Progress
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir at revision 516, scheduled for release in mir, milestone 0.0.2

Changed in mir:
status: In Progress → Fix Committed
Changed in mir:
milestone: none → 0.0.3
Changed in mir:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.