[PATCH] Use libpcl for portable coroutines

[Sam Liddicott]

* Stefan (metze) Metzmacher wrote, On 15/05/09 12:52:
>
> We may need to experiment with this.
>
> But with the tevent_coroutine design (where only the main coroutine body
> can call yield) I can't see how we can end up with a large stack trace.
> As long as don't call recursive function and don't use large buffer
> variables on the stack.
>
> A self growing stack would be perfect, but maybe not really needed.
> (Maybe we can add that to libpcl)
>   
The author wasn't particularly interested.
>> It took me a while to grasp the example at:
>> http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=1efdd2469b7ae2b4c0dd4e78ec7c864b3cf47faa;hp=ced03d86f39a5f87af682f48defef5fc73d61388
>> but it seems that the main principle is that you can nest co-routines
>> providing each inner one is given a completion callback by it's creator
>> that switches back to it's creators stack.
>>
>> The risk seems to be in ensuring that the creator stack isn't activated
>> before the thing it waits on is complete - or that it can tell if this
>> occurs and switches to co_main to await the official callback.
>>
>> I think co_resume() isn't generally safe to call - co_resume() doesn't
>> neccessarily  switch to the creator coroutine which might be useful. If
>> a co-routine was launched from another coroutine then the two coroutines
>> may keep flipping between each-other, which is why I switched to
>> co_call(co_main) which in our case is the stack running the event loop in.
>> I gnash my teeth that the real co_main is static - so I had to
>> "discover" with co_current() when the first co_create() is called by
>> wrapping co_create().
>>     
>
> I changed my code a little bit to make sure we never nest coroutines.
>   

I think nesting co-routines is as safe as nesting composites (which we
do a lot of), I don't know why you would want to stop this.

> http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-tevent-coro-2
> http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=f6c3ea8784156e3697c3930fe46cb2c49aba37d2
> http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=dba77a388d644828c17f816f88645e2bfd90230d
>
> tevent_coroutine_run() is now static and only every triggered from
> the main event loop. (See the immediate trigger event).
>
> The example_coro_send() only schedules the immediate trigger event.
>
>   
>> I think it's a shame that the active tevent_coroutine needs to be passed
>> to the yield function,
>>     
>
> It's not a shame, it's really good, because it only allows the main
> coroutine body to call tevent_coroutine_yield(). It makes
> the yield explicit and avoids implicit side effects.
>   

One of the main reasons I wrote my patch is for the implicit side
effects, which I go more into below.

>> but I see we must unless we extend libpcl to
>> allow user data to be attached to coroutine_t. Maybe we could nick the
>> first few bytes from the stack to point to the tevent_cououtine? I think
>> if we may be linking to other libraries that also use libpcl that
>> perhaps we must do as you have done...
>>
>> I also prefer (as you saw in my patch) that tevent_coroutine_yield
>> should be called implicitly by example_ocor_recv.
>>     
>
> No! This is exactly what leads to nested event loops.
>   
No it doesn't, I don't think you looked at my patch.

If the main stack is not active, then the nested event loop is not
called - which will be the case when called from a co-routine. This is
what avoids nested event loops. The only reason I didn't remove the
nested event code altogether is because some code still depends on it,
and if it is called from the main stack it is the only possible thing to do.

> We should have this explicit for a "sync" call driven
> by a coroutine:
>
> subreq = foo_send(mem_ctx, event_ctx);
> if (!tevent_coroutine_yield(subreq, tco)) {
> 	return tevent_coroutine_return(tco);
> }
> result = foo_recv(subreq);
>   

I don't like it for the reason already given below.

>> One consequence is
>> that code can be written synchronously without much thought for
>> co-routines, but work asynchronously as co-routines if support is
>> available. Also sometimes the _send and _recv functions may be hidden
>> behind an api and the caller might not have anything on which to yield.
>>     
>
> Then it's a real sync function that doesn't need an event context
> or it's a bug that needs to be fixed.
>   
The point is, it's a sync function (for laziness and clarity) that
doesn't block when called from a co-routine.
It's not a "bug" it's the whole point of it; it's the incentive for
callers of the function to convert to being a co-routine.

[good news about merging of async structs deleted]

>> In the example I think the *_send() are misnamed because unlike the old
>> async pattern, they don't send, they are just a trampoline launch the
>> co-routine.
>>     
>
> Now the _send function schedules the execution of the body function.
>   
That's what I was saying.
(more below)

>> I dearly want to get rid of the *_send() functions in your example - the
>> fact that they exist at all is because there isn't a portable way to
>> replicate the current stackframe onto the new stack, otherwise co_create
>> could work like fork(). As this isn't possible, I think that this
>> trampoline should be minimized as much as possible as it is a
>> distraction from the intent of the code.
>>
>> I tried to keep the trampoline to be nothing beyond the necessary
>> declarations:
>>
>> void proxy_connect_trampoline(va_list args)
>> {
>>     co_get_va(struct ntvfs_module_context *, ntvfs);
>>     co_get_va(struct ntvfs_request *, req);
>>     co_get_va(union smb_tcon *, tcon);
>>
>>     ntvfs_co_passthrough_exit(proxy_connect(ntvfs, req, tcon));
>> }
>>
>> but yet the trampoline here (by means of the passthrough macro) also
>> deals with co_exit and other cleaning up. The body function can then be
>> just a regular synchronous function.
>>
>>     
>
> That's too much magic for my taste.
>   
The send function is a trampoline for the body function and yet the body
function still has to contain code like:

       tevent_coroutine_done(coro);
       return tevent_coroutine_return(coro);

which seems like the worst of both worlds in that the trampolines are
very verbose and the body can only be called as a new co-routine, and
the intent of the function is dreadfully combined with the
implementation of the co-routines.

You still require the magic but you require it to be incanted
word-perfect every time.

The co-routine body does not need to have any special co-routine stuff
inside it unless it wants to do a benevolent yield, everything else can
be in the trampoline function.
> We should have only one way to call async function and that's
> with tevent_req based _send/_recv pair. 

Maybe, but I'm talking about sync functions that are called from an
async function.

With my patch, an async function can call smbcli_nt_create_full() or any
other sync function without blocking the process. It can also suspend
and be resumed based on any simple function/data callback which doesn't
derive from a simple tevent.

> And the caller really
> should not need to know how the internals look like.
>   

The author still has to, and the caller has to write the invocation.

I wrote my patch with the user in mind; a main-thread function can call
a regular synchronous function like this:
  do_something(ntvfs, req, io);

or asynchronously like this:
  co_create_va(NULL, 16384, do_something_trampoline, ntvfs, req, io);

and if called asynchronously, then it behaves asynchronously "by magic"
because the _recv functions all yield when called from a co-routine.

Otherwise we have to start having two of every function, the async
version which looks just like the sync version apart from a strategic
scattering of unnecessary co-routine oil. (And an async function can't
go and call the async version of another function if co-routines can't
be nested)

>   
>> Perhaps we have different needs in this work.
>>
>> It's good that tevent integrates nicely with co-routines, but I think
>> that co-routines here are integrating too tightly with tevent, and
>> becoming too dependant on it. I think the private_data and co_call
>> routine should be the key interface to activating a co-routine, and not
>> tevent; partly because this IS the interface that already exists with
>> composites, ntvfs_requests and smbcli_requests.
>>     
>
> I don't see the problem here, see my example how ntvfs_requests and
> tevent_req based coroutines can interact fine. In the long term
> composite_context, ntvfs_request and others should be converted
> to tevent_req.
>   

The main problem is as I suggested, your solution fits your needs very
well but not mine at all.
Not all continuations are from a pre-known tevent, but perhaps a
calculated state.

>> I think that co_main is fundamental to co-routine use here. A function
>> may call another function asynchronously by launching the co-routine
>> before calling, or it may permit the other function to do this
>> internally. Whichever function launches the co-routine must also set the
>> handler to switch back on completion, but the special case arises when
>> the co-routine is launched from the main stack.
>>
>> If the main stack launches a co-routine, then it must return signifying
>> that a response will follow later.
>> If one co-routine launches another, then the launching co-routine is
>> suspended until the other returns, and the invoking caller from the main
>> stack already expects an asynchronous response.
>>
>> Are there cases where one co-routine will launch another, and expect to
>> be returned to before the other has completed? There may if it wants to
>> parallelize requests. It will have to launch all co-routines and then
>> enter a loop that keeps co_call(co_main) until all requests are
>> complete. It does not need to use tevent directly, as long as the
>> requests it is waiting for use tevent.
>>     
>
> As every async function needs tevent, I think it makes no sense to
> decouple both.
>   
I think the items used in that reasoning are not relevant to the decision.

Even if every async function needs a tevent, it can still make could
sense not to make the co-routines dependant on tevent.

It further may not be true that every async function needs a tevent (or
has one available) [1]
and even if it is true, it is not always simple what the "wake up"
situation ought to be,

[1] you could call it a bug, but it someone would have to use this:
{
  union smb_open open_parms;

  open_parms.ntcreatex.level = RAW_OPEN_NTCREATEX;
  open_parms.ntcreatex.in.flags = 0;
  open_parms.ntcreatex.in.root_fid = 0;
  open_parms.ntcreatex.in.access_mask = SEC_FILE_READ_DATA;
  open_parms.ntcreatex.in.file_attr = FILE_ATTRIBUTE_NORMAL;
  open_parms.ntcreatex.in.alloc_size = 0;
  open_parms.ntcreatex.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
  open_parms.ntcreatex.in.open_disposition = NTCREATEX_DISP_OPEN;
  open_parms.ntcreatex.in.create_options = NTCREATEX_OPTIONS_DIRECTORY;
  open_parms.ntcreatex.in.impersonation = 0;
  open_parms.ntcreatex.in.security_flags =
NTCREATEX_IMPERSONATION_IMPERSONATION;
  open_parms.ntcreatex.in.fname = filename;

  struct smbcli_request *creq = smb_raw_open_send(private->tree,
&open_parms);
  if (!tevent_smbcli_yield(creq, tco)) {
    return tevent_coroutine_return(tco);
  }
  status = smb_raw_open_recv(creq, req, &open_parms);

  if (NT_STATUS_IS_OK(status)) {
    nttrans_fnum = open_parms.ntcreatex.out.file.fnum;
  } else {
    nttrans_fnum = -1;
  }


instead of this:

  nttrans_fnum=smbcli_nt_create_full(private->tree, filename,
    0,
    SEC_FILE_READ_DATA,
    FILE_ATTRIBUTE_NORMAL,
    NTCREATEX_SHARE_ACCESS_MASK,
    NTCREATEX_DISP_OPEN,
    NTCREATEX_OPTIONS_DIRECTORY,
    NTCREATEX_IMPERSONATION_IMPERSONATION);

and I'm really thinking of some examples in ntvfs_generic where sync
smbcli functions are issued in the reverse map functions, because it is
too hard to do async. Your patch doesn't make that task much easier, but
my patch automatically asyncs those sync functions.
> As said above() with my code we only every switch from the
> main stack to a coroutine and back. We never switch between
> coroutines directly.
>   
You say this, but you use co_resume() and thats not what co_resume promises.
This might be true providing none of the libraries you link to also link
with libpcl.

But this is an unnecessary constraint you place in order to use
co_resume() wrongly, and perhaps you place it because you are so tightly
linked to tevent that you think you won't activate one co-routine from
another.

I'm planning to activate one co-routine from another; it's quite safe
for one co-routine to spawn another and then pass it's own activation
function (and pointer) as the completion method for the new co-routine.
Both co-routines will suspend back to the main stack when the inner
routine does it's yield.

>> So these are my thoughts;
>> * that it should be simple to launch a new coroutine without using tevent
>>     
>
> That doesn't make sense to me.
>   

That's because for you everything uses tevent, but sometimes the
continuation of a coroutine isn't based on a tevent that can be known
before hand.

>   
>> * that one day composites, smbcli_requests, ntvfs_requests and
>> tevent_req will share a common async union
>>     
>
> tevent_req is that union, the other's should went away in future.
>   
excellent.
>   
>> * that trampolines be kept as tiny as C allows, and allow the body to be
>> written as a regular sync function
>> * that _recv functions do the yielding so the caller doesn't have to
>> (saves re-writing lots of code)
>>     
>
> For long term, we have to rewrite the code.
> Which we have to do any way, because the really bad bugs with broken
> connections.
>   
Maybe, but I can't see what that has to do with the _recv functions
(unless you are still thinking that they use a nested event loop).

Sam